FIX: LAMMPSDUMP Convert forces from kcal to kJ units by Rupesh-Singh-Karki · Pull Request #5117 · MDAnalysis/mdanalysis

Rupesh-Singh-Karki · 2025-09-27T19:46:39Z

Changes made in this Pull Request:

Add units attribute to DumpReader class specifying LAMMPS "real" units (time: fs, length: Angstrom, velocity: Angstrom/fs, force: kcal/(mol*Angstrom))
Implement force unit conversion in _read_next_timestep() method to convert forces from native kcal/(molAngstrom) to MDAnalysis base units kJ/(molAngstrom)
Add unit conversion calls for positions, velocities, and forces when convert_units=True (default behavior)
Update docstring to document the automatic force unit conversion behavior
Add comprehensive tests for force unit conversion functionality:
- Test units attribute definition
- Test conversion factor calculation (4.184)
- Test actual force conversion using existing test files
- Test conversion consistency across all trajectory frames
Ensure backward compatibility with convert_units=False option

This fix resolves the inconsistency where LAMMPSDUMP forces remained in kcal/(molAngstrom) while other trajectory formats (GROMACS TRR, AMBER NetCDF) properly converted forces to kJ/(molAngstrom). Now all trajectory formats use consistent force units for downstream analysis.

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

📚 Documentation preview 📚: https://mdanalysis--5117.org.readthedocs.build/en/5117/

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on GitHub Discussions so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2025-09-27T19:57:23Z

Codecov Report

❌ Patch coverage is 46.93878% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.82%. Comparing base (519ac56) to head (518bca9).
⚠️ Report is 27 commits behind head on develop.

Files with missing lines	Patch %	Lines
package/MDAnalysis/coordinates/LAMMPS.py	46.93%	18 Missing and 8 partials ⚠️

❗ There is a different number of reports uploaded between BASE (519ac56) and HEAD (518bca9). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (519ac56) HEAD (518bca9)

6 3

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5117      +/-   ##
===========================================
- Coverage    93.88%   85.82%   -8.07%     
===========================================
  Files          180      180              
  Lines        22422    22496      +74     
  Branches      3186     3202      +16     
===========================================
- Hits         21052    19308    -1744     
- Misses         902     2722    +1820     
+ Partials       468      466       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

orbeckst · 2025-09-30T22:18:58Z

@hmacdope @jaclark5 could you review this PR? I think you both have some LAMMPS experience. Or can you think of someone else?

orbeckst · 2025-09-30T22:20:23Z

@hmacdope I have tentatively assigned you as PR shepherd. If you don't have the bandwidth, please find someone else or un-assign yourself and let me know. Thanks.

tylerjereddy

This does seem reasonably solid at first glance:

no old tests were modified, only new tests added
3/4 of the new tests fail before the new source patch is applied, and all pass after

I made a few small suggestions. I'm not a LAMMPS expert (though it is heavily used here).

tylerjereddy · 2025-10-04T03:09:14Z

testsuite/MDAnalysisTests/coordinates/test_lammps.py

+
+        assert DumpReader.units == expected_units
+
+    def test_force_unit_conversion_factor(self):


This is the only test that doesn't fail when the source patch in this PR is reverted.

This test appears to be now passing?

testsuite/MDAnalysisTests/coordinates/test_lammps.py

tylerjereddy · 2025-10-04T03:12:19Z

testsuite/MDAnalysisTests/coordinates/test_lammps.py

+
+        # 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)


Hmm, this check is perhaps a bit less convincing. What about checking the first and last force with LAMMPS or another reader library and ensuring those are preserved in MDA with convert_units=False?

This is assuming that the forces in native units are zero? Consider using assert_allclose with the forces values copied directly from the dump file

jaclark5

Thanks for doing this! It looks like these changes would commit the DumpReader to expecting real units, however, lj and metal are also very common options. You're going to have to implement the timeunit and lengthunit keywords and then introduce an energy keyword energyunit. This would be consistent with the LAMMPS.DCDReader and then the documentation accordingly:

        self.units["time"] = kwargs.pop("timeunit", self.units["time"])
        self.units["length"] = kwargs.pop("lengthunit", self.units["length"])

… conversion, and native-force preservation test

…zable

jaclark5

The changes look great. Only ~40% of the lines of code are covered by these tests, once that's up to 100% and we go through the tests I think it'll be ready

jaclark5 · 2025-10-15T23:42:15Z

testsuite/MDAnalysisTests/coordinates/test_lammps.py

+
+        # 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)


This is assuming that the forces in native units are zero? Consider using assert_allclose with the forces values copied directly from the dump file

jaclark5 · 2025-10-15T23:45:03Z

testsuite/MDAnalysisTests/coordinates/test_lammps.py

+
+        assert DumpReader.units == expected_units
+
+    def test_force_unit_conversion_factor(self):


This test appears to be now passing?

orbeckst · 2025-10-16T20:08:37Z

testsuite/MDAnalysisTests/analysis/conftest.py

-# MDAnalysis.analysis.lineardensity
-
-
-@pytest.fixture(scope="module", params=params_for_cls(LinearDensity))


Don't remove that.

orbeckst · 2025-10-16T20:09:03Z

testsuite/MDAnalysisTests/analysis/conftest.py

Don't change anything in this file unless you have to.

FIX: LAMMPSDUMP Convert forces from kcal to kJ units

3c3c81c

github-actions bot reviewed Sep 27, 2025

View reviewed changes

Fixed failing lint tests

0671174

orbeckst requested review from hmacdope and jaclark5 September 30, 2025 22:18

orbeckst assigned hmacdope Sep 30, 2025

tylerjereddy added the format-LAMMPS label Oct 4, 2025

tylerjereddy reviewed Oct 4, 2025

View reviewed changes

jaclark5 requested changes Oct 9, 2025

View reviewed changes

Rupesh-Singh-Karki and others added 18 commits October 9, 2025 09:26

LAMMPS DumpReader: unit overrides (time/length/energy), default force…

518bca9

… conversion, and native-force preservation test

mark analysis.rdf.InterRDF and analysis.rdf.InterRDF_s as not paralli…

e5ee9ac

…zable

Adds parallization for analysis.rdf.InterRDF

f960696

Minor changes

04ee5ae

Adds custom aggregegator for InterRDF_s

4887d03

Fixes aggregation of results.edges

9af7feb

Parallizes InterRDF_s

080fdae

Minor changes

7b59148

Fixes linter

89d98e3

Fixes linter

74c83f2

Tests for parallization

e517777

refactor custom aggregator for rdf

4fe2f50

remove uneccesary variables

8ef74d8

Update CHANGELOG

77435a4

adds tests for nested_array_sum

740084a

fixes formatting

eeeccb5

fixes formatting

530db92

Remove unused code from test_nested_array_sum

f0e1060

p-j-smith and others added 4 commits October 9, 2025 09:49

remove unnecessary tests from test_rdf and test_rdf_s

aba969f

Make linters happy

cd1fc7f

Merge branch 'develop' into fix/convert-units-of-trajectory

4785029

Merge branch 'develop' into fix/convert-units-of-trajectory

930eb12

jaclark5 requested changes Oct 15, 2025

View reviewed changes

jaclark5 mentioned this pull request Oct 16, 2025

Support for LAMMPS LJ reduced units #5009

Open

orbeckst reviewed Oct 16, 2025

View reviewed changes


		assert DumpReader.units == expected_units

		def test_force_unit_conversion_factor(self):

		# MDAnalysis.analysis.lineardensity


		@pytest.fixture(scope="module", params=params_for_cls(LinearDensity))

Conversation

Rupesh-Singh-Karki commented Sep 27, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Developers Certificate of Origin

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst commented Sep 30, 2025

Uh oh!

orbeckst commented Sep 30, 2025

Uh oh!

tylerjereddy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaclark5 left a comment

Choose a reason for hiding this comment

Uh oh!

jaclark5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Rupesh-Singh-Karki commented Sep 27, 2025 •

edited by github-actions bot

Loading

codecov bot commented Sep 27, 2025 •

edited

Loading