-
Notifications
You must be signed in to change notification settings - Fork 764
FIX: LAMMPSDUMP Convert forces from kcal to kJ units #5117
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
3c3c81c
0671174
518bca9
e5ee9ac
f960696
04ee5ae
4887d03
9af7feb
080fdae
7b59148
89d98e3
74c83f2
e517777
4fe2f50
8ef74d8
77435a4
740084a
eeeccb5
530db92
f0e1060
aba969f
cd1fc7f
4785029
930eb12
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 |
|---|---|---|
|
|
@@ -768,6 +768,100 @@ def test_warning(self, system, request): | |
| with pytest.warns(match="Some of the additional"): | ||
| request.getfixturevalue(system) | ||
|
|
||
| def test_dump_reader_units_attribute(self): | ||
| """Test that DumpReader has proper units defined""" | ||
| from MDAnalysis.coordinates.LAMMPS import DumpReader | ||
|
|
||
| expected_units = { | ||
| 'time': 'fs', | ||
| 'length': 'Angstrom', | ||
| 'velocity': 'Angstrom/fs', | ||
| 'force': 'kcal/(mol*Angstrom)' | ||
| } | ||
|
|
||
| assert DumpReader.units == expected_units | ||
|
|
||
| def test_force_unit_conversion_factor(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. This is the only test that doesn't fail when the source patch in this PR is reverted.
Contributor
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 appears to be now passing? |
||
| """Test that the force conversion factor is correct""" | ||
| from MDAnalysis import units | ||
jaclark5 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| # Get conversion factor from kcal/(mol*Angstrom) to kJ/(mol*Angstrom) | ||
| factor = units.get_conversion_factor( | ||
| 'force', | ||
| 'kcal/(mol*Angstrom)', # from (LAMMPS native) | ||
| 'kJ/(mol*Angstrom)' # to (MDAnalysis base) | ||
| ) | ||
|
|
||
| expected_factor = 4.184 # 1 kcal = 4.184 kJ | ||
| assert_allclose(factor, expected_factor, rtol=1e-6) | ||
|
|
||
| def test_force_conversion_with_image_vf_file(self): | ||
| """Test force unit conversion using existing test file with forces""" | ||
| # Test with convert_units=True (default) | ||
| u_converted = mda.Universe( | ||
| LAMMPS_image_vf, | ||
| LAMMPSDUMP_image_vf, | ||
| format="LAMMPSDUMP" | ||
| ) | ||
|
|
||
| # Test with convert_units=False | ||
| u_native = mda.Universe( | ||
| LAMMPS_image_vf, | ||
| LAMMPSDUMP_image_vf, | ||
| format="LAMMPSDUMP", | ||
| convert_units=False | ||
| ) | ||
|
|
||
| # Both should have forces | ||
| assert hasattr(u_converted.atoms, 'forces') | ||
| assert hasattr(u_native.atoms, 'forces') | ||
|
|
||
| # Go to last frame where we know forces exist | ||
| u_converted.trajectory[-1] | ||
| u_native.trajectory[-1] | ||
|
|
||
| forces_converted = u_converted.atoms.forces | ||
| forces_native = u_native.atoms.forces | ||
|
|
||
| # Check that forces are different (converted vs native) | ||
| # The conversion factor should be 4.184 | ||
| expected_factor = 4.184 | ||
| forces_expected = forces_native * expected_factor | ||
|
|
||
| # Test that converted forces match expected values | ||
| assert_allclose(forces_converted, forces_expected, rtol=1e-6) | ||
|
|
||
| # Test that native forces are unchanged when convert_units=False | ||
| # Just check they are reasonable values (not zero everywhere) | ||
| assert not np.allclose(forces_native, 0.0) | ||
|
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. Hmm, this check is perhaps a bit less convincing. What about checking the first and last force with
Contributor
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 is assuming that the forces in native units are zero? Consider using |
||
|
|
||
| def test_force_conversion_consistency_across_frames(self): | ||
| """Test that force conversion works consistently across all frames""" | ||
| u_converted = mda.Universe( | ||
| LAMMPS_image_vf, | ||
| LAMMPSDUMP_image_vf, | ||
| format="LAMMPSDUMP" | ||
| ) | ||
|
|
||
| u_native = mda.Universe( | ||
| LAMMPS_image_vf, | ||
| LAMMPSDUMP_image_vf, | ||
| format="LAMMPSDUMP", | ||
| convert_units=False | ||
| ) | ||
|
|
||
| conversion_factor = 4.184 | ||
|
|
||
| # Test conversion in all frames | ||
| for ts_conv, ts_native in zip(u_converted.trajectory, u_native.trajectory): | ||
| if ts_conv.has_forces: | ||
| forces_converted = ts_conv.forces | ||
| forces_native = ts_native.forces | ||
| forces_expected = forces_native * conversion_factor | ||
|
|
||
| assert_allclose(forces_converted, forces_expected, rtol=1e-6, | ||
| err_msg=f"Force conversion failed at frame {ts_conv.frame}") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "convention", ["unscaled", "unwrapped", "scaled_unwrapped"] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.