Add mass; add the definition of molecular orientation - #84
Conversation
V-Francois
left a comment
V-Francois
left a comment
There was a problem hiding this comment.
Thanks for the great PR Danqi.
I have only a few minor comments, mostly about making a few things more explicit.
Also, all the checks that test that indices are in bounds, magnitudes not too low, etc. are great for now, to help make sure there is no hidden bug.
Eventually this will not be required, and just slow down things. I created issue #85 to remember to remove this in the future.
There was a problem hiding this comment.
That's a useful document, but I think it's better to not include this in the code base.
You can compile it on your side, then attach the PDF to the PR description, this way it's not lost, but it doesn't become part of the code
| centre of mass to local atom `atom`. | ||
| """ | ||
| struct CenterToAtomOrientation <: OrientationDefinition | ||
| atom::Int |
There was a problem hiding this comment.
I would add a comment to say that this atom index is in the "Molecular frame". i.e. if atom=1, it's the first atom of the molecule, which is not the first atom of the system (except for the first molecule).
| reverses the orientation. | ||
| """ | ||
| struct PlaneNormalOrientation <: OrientationDefinition | ||
| atom1::Int |
There was a problem hiding this comment.
Same comment as for the CenterToAtomOrientation class above.
| return sum(mass * position for (mass, position) in zip(masses, unwrapped)) / total_mass | ||
| end | ||
|
|
||
| """Compute an orientation from an explicit collection of molecular positions.""" |
There was a problem hiding this comment.
I would also add in the description that here, the positions given the function, must be the positions of the molecule only, not of the whole system, to be consistent with the definition of the atoms in the class.
This is what your code does, but it might not be obvious to a reader, because here the array positions is a slice/view of the vector of the same name of the system object.
| return _normalise_orientation(unwrapped[definition.atom] - center) | ||
| end | ||
|
|
||
| function orientation(definition::PlaneNormalOrientation, |
| @view(system.mass[first_atom:last_atom]), | ||
| system.box, | ||
| ) | ||
| end |
There was a problem hiding this comment.
It looks like this function is never used.
It's a relatively natural definition, so I guess you can keep it, it might be useful in the future.
I would however move it so that both definitions of molecule_center_of_mass are one after the other in the code.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
2 participants
Summary
System,load_chains, and TOML input.Notes
This PR implements molecular orientation geometry only. Coupling the
orientation to an external-field Hamiltonian will be introduced separately.