-
Notifications
You must be signed in to change notification settings - Fork 853
Rotate velocities and forces besides positions during trajectory rotation #5452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
d5e83dc
df93377
00f4db2
fd257eb
e99d6d9
5cd7c4e
daa0a7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -417,6 +417,34 @@ def test_rotateby(self, u, coords): | |
| [-2 * np.cos(angle) + 1, -2 * np.sin(angle), 0], | ||
| ) | ||
|
|
||
| def test_rotate_velocities_forces(self): | ||
|
orbeckst marked this conversation as resolved.
|
||
| u = mda.Universe.empty( | ||
| 2, trajectory=True, velocities=True, forces=True | ||
| ) | ||
| u.atoms.positions = np.array([[1, 0, 0], [-1, 0, 0]]) | ||
| u.atoms.velocities = np.array([[1, 0, 0], [0, 1, 0]]) | ||
| u.atoms.forces = np.array([[0, 0, 1], [1, 1, 0]]) | ||
|
|
||
| orig_v = u.atoms.velocities.copy() | ||
| orig_f = u.atoms.forces.copy() | ||
|
|
||
| axis = np.array([0, 0, 1]) | ||
| for angle in np.linspace(0, np.pi): | ||
| R = transformations.rotation_matrix(angle, axis)[:3, :3] | ||
| u.atoms.velocities = orig_v.copy() | ||
| u.atoms.forces = orig_f.copy() | ||
| u.atoms.rotate(R) | ||
| assert_almost_equal(u.atoms.velocities, np.dot(orig_v, R.T)) | ||
| assert_almost_equal(u.atoms.forces, np.dot(orig_f, R.T)) | ||
|
|
||
| def test_rotate_no_velocities_forces_does_not_raise(self): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this test testing? Is it necessary?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test (and the similar one in The original version only checked 'doesn't crash'. To strengthen these tests, I've added a position assertion so it also confirms rotation still works correctly in that case.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about the other tests that were present before? Don't these tests already cover this case naturally?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your suggestion @orbeckst . I checked, and the fixtures used by test_rotate/test_rotateby are already covering the case; my tests were duplicating that coverage. I'll remove these tests. |
||
| u = mda.Universe.empty(2, trajectory=True) | ||
| orig_pos = np.array([[1, 0, 0], [-1, 0, 0]]) | ||
| u.atoms.positions = orig_pos.copy() | ||
| R = transformations.rotation_matrix(1, [-1, 2, -3])[:3, :3] | ||
| u.atoms.rotate(R) | ||
| assert_almost_equal(u.atoms.positions, np.dot(orig_pos, R.T)) | ||
|
|
||
| def test_transform_rotation_only(self, u, coords): | ||
| R = np.eye(3) | ||
| u.atoms.rotate(R) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,6 +182,46 @@ def test_rotateby_atomgroup_com_pbc(rotate_universes): | |
| assert_array_almost_equal(transformed.positions, ref.positions, decimal=6) | ||
|
|
||
|
|
||
| def test_rotateby_velocities_forces(): | ||
| u = mda.Universe.empty(2, trajectory=True, velocities=True, forces=True) | ||
| u.atoms.positions = np.array([[1, 0, 0], [-1, 0, 0]]) | ||
| u.atoms.velocities = np.array([[1, 0, 0], [0, 1, 0]]) | ||
| u.atoms.forces = np.array([[0, 0, 1], [1, 1, 0]]) | ||
| ts = u.trajectory.ts | ||
|
|
||
| orig_v = ts.velocities.copy() | ||
| orig_f = ts.forces.copy() | ||
|
|
||
| axis = [-1, 2, -3] | ||
| point = [0, 0, 0] | ||
| angle = 23 | ||
| matrix = rotation_matrix(np.deg2rad(angle), axis, point) | ||
| rotation = matrix[:3, :3].T | ||
|
|
||
| transformed_ts = rotateby(angle, axis, point=point)(ts) | ||
|
|
||
| assert_array_almost_equal( | ||
| transformed_ts.velocities, np.dot(orig_v, rotation), decimal=6 | ||
| ) | ||
| assert_array_almost_equal( | ||
| transformed_ts.forces, np.dot(orig_f, rotation), decimal=6 | ||
| ) | ||
|
|
||
|
|
||
| def test_rotateby_no_velocities_forces_does_not_raise(): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question above, is this a necessary test?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason as for |
||
| u = mda.Universe.empty(2, trajectory=True) | ||
| orig_pos = np.array([[1, 0, 0], [-1, 0, 0]]) | ||
| u.atoms.positions = orig_pos.copy() | ||
| ts = u.trajectory.ts | ||
| angle = 5 | ||
| matrix = rotation_matrix(np.deg2rad(angle), [-1, 2, -3], [0, 0, 0]) | ||
| rotation = matrix[:3, :3].T | ||
| transformed_ts = rotateby(angle, [-1, 2, -3], point=[0, 0, 0])(ts) | ||
| assert_array_almost_equal( | ||
| transformed_ts.positions, np.dot(orig_pos, rotation), decimal=6 | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "ag", | ||
| ( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be formatted correctly. You need two blank lines (quirk of reST and the Napoleon parser) and you need to indent properly:
Always check the readthedocs check that the docs get rendered properly, see eg https://mdanalysis--5452.org.readthedocs.build/en/5452/documentation_pages/core/groups.html#MDAnalysis.core.groups.AtomGroup.rotate where the versionchanged is not visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please also add the versionchanged to the docs for
rotatebyhttps://mdanalysis--5452.org.readthedocs.build/en/5452/documentation_pages/core/groups.html#MDAnalysis.core.groups.AtomGroup.rotateby — you didn't change that one but from a user's perspective it's not obvious. Please check that these docs also render properly. Thanks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for missing out the doc update in
rotatebyand the indentation. I've update the docs and ensured they render properly.