Fix incorrect unit conversion factors in units.py#5053

raulloiscuns · 2025-05-28T11:09:03Z

Changes made in this Pull Request:

The conversion factors for A/fs, A/ms and A/us have been revised and corrected in units.py.

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--5053.org.readthedocs.build/en/5053/

… cases

…into develop

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.

…cities for test consistency

codecov · 2025-06-05T10:47:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.67%. Comparing base (19e675b) to head (e9f24c2).
⚠️ Report is 24 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5053      +/-   ##
===========================================
- Coverage    92.68%   92.67%   -0.01%     
===========================================
  Files          180      180              
  Lines        22452    22452              
  Branches      3186     3186              
===========================================
- Hits         20810    20808       -2     
- Misses        1167     1170       +3     
+ Partials       475      474       -1

☔ 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

Thank you VERY much for finding this very old bug. I assume you found it while working with LAMMPS files? Great work fixing and also updating the tests.

Please address the following points (see inline comments for details)

update CHANGELOG (include your GitHub handle and rewrite/add entries a bit)
please add Å/nm to the conversions (I will merge your PR if you don't do it but it would be nice as it fits directly in with your fix)
please remove the formatting change to the LAMMPS writer, we'd like to keep PRs focused on one thing

Apologies that it took a while to review your PR.

orbeckst · 2025-06-11T07:12:17Z

package/MDAnalysis/coordinates/LAMMPS.py

        for index, vel in zip(indices, velocities):
            self.f.write(
-                "{i:d} {x:f} {y:f} {z:f}\n".format(
+                "{i:d} {x:.8f} {y:.8f} {z:.8f}\n".format(


This may be useful but does not belong into this PR. Please revert and make a separate PR – then people knowledgable about LAMMPS can discuss.

It seems that the writer test does not pass unless we force higher precision. Our LAMMPS test files have 10 decimals so I am tentatively setting all output to .10f.

package/MDAnalysis/units.py

package/CHANGELOG

testsuite/MDAnalysisTests/utils/test_units.py

orbeckst · 2025-06-26T20:36:51Z

@raulloiscuns it would be great to add your fix to MDAnalysis soon. Are you able to address the review comments?

- updated table of units - added tests

orbeckst

I am finishing the PR so that we can include it in the 2.10.0 release.

orbeckst · 2025-10-14T00:37:15Z

package/MDAnalysis/coordinates/LAMMPS.py

        for index, vel in zip(indices, velocities):
            self.f.write(
-                "{i:d} {x:f} {y:f} {z:f}\n".format(
+                "{i:d} {x:.8f} {y:.8f} {z:.8f}\n".format(


package/CHANGELOG

package/MDAnalysis/units.py

testsuite/MDAnalysisTests/utils/test_units.py

IAlibay

I'm not reviewing this PR, but please do order the changelog entries properly to save me time on release.

IAlibay · 2025-10-14T00:46:37Z

package/CHANGELOG

 * Fixes the benchmark `SimpleRmsBench` in `benchmarks/analysis/rms.py`
   by changing the way weights for RMSD are calculated, instead of
   directly passing them. (Issue #3520, PR #5006)
+ * Fixes incorrect conversion factors for speed units A/fs, A/us, A/ms


Please put things done in this PR at the top of the entry list.

ok, I'll also move the entry from PR #5113 up

orbeckst · 2025-10-14T00:48:18Z

I am using pre-commit with black 24 and it passes and then our linters/black fails; I probably need to sync the settings in some way, until then there'll be more micro commits of the type "blackened file xxx" (which I'll squash merge).

IAlibay

I might as well actually review rather than just lurk.

package/MDAnalysis/coordinates/LAMMPS.py

package/MDAnalysis/units.py

IAlibay · 2025-10-14T00:59:42Z

package/CHANGELOG

Not this PR, just a general comment. We should probably come up with some kind of rule about autoformatting the changelog, personally not sure it makes sense to have black apply to it since it's just free-form text.

Sorry, didn't see that it removed all the trailing whitespace.

This is my fault for not configuring pre-commit hook properly (or disabling it). It won't let me commit until it's happy and I may just not have the right .pre-commit-config.yaml.

I think it wasn't black that touched CHANGELOG because when I only committed CHANGELOG, black did not engage

(mda-develop) mac:MDAnalysisTests oliver$ git commit --amend black................................................(no files to check)Skipped flake8...............................................(no files to check)Skipped trim trailing whitespace.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check for merge conflicts................................................Passed

Instead it's likely the trim trailing whitespace plugin for pre-commit.

It would be great to have an expert share a pre-commit yaml file for MDA development.

testsuite/MDAnalysisTests/utils/test_units.py

IAlibay · 2025-10-14T01:09:30Z

I am using pre-commit with black 24 and it passes and then our linters/black fails; I probably need to sync the settings in some way, until then there'll be more micro commits of the type "blackened file xxx" (which I'll squash merge).

cc @RMeli

- add tests for unit conversion with unicode characters - add additional representations Å/us, Å/µs, Å/fs, Å/ms

orbeckst · 2025-10-14T01:43:17Z

package/MDAnalysis/coordinates/LAMMPS.py

Sorry for the noise. When I reverted a change, pre-commit/flake/black were all unhappy about the file so I did some line breaking and a single real fix.

package/MDAnalysis/coordinates/LAMMPS.py

IAlibay

lgtm, thanks!

- match precision in our test files - required for tests to pass when round-tripping velocities - updated CHANGELOG

orbeckst · 2025-10-14T16:34:43Z

Thanks @IAlibay , just updated CHANGELOG with output precision changes and ran black locally again. I think I just don't have the right settings for pre-commit black and our black and need to sort that out.

If the latest changes pass our CI, I'll also approve and squash-merge. Thanks a lot for your extra 👀 — much appreciated!

orbeckst · 2025-10-14T17:01:32Z

(This got automerged on approval... I had thought it would wait for all of CI... oops, sorry. All linters and Linux had passed at least. If anything gets messed up on develop I'll fix it.)

orbeckst · 2025-10-14T19:29:13Z

@raulloiscuns your PR got merged and your fix will be in 2.10.0. 🎉 Thank you very much for identifying the problem and proposing a solution!

RMeli · 2025-10-15T20:44:23Z

Sorry @orbeckst and @IAlibay, I just noticed the ping. But looks like the linters passed, do you know what changed in order to fix things?

orbeckst · 2025-10-15T21:10:36Z

I am using the following .pre-commit.yml

Details

# Pre-commit configuration for MDAnalysis
repos:
  # Black - Python code formatter
  - repo: https://github.com/psf/black
    # workaround https://github.com/psf/black/issues/2493#issuecomment-1081987650
    rev: 'refs/tags/24.10.0:refs/tags/24.10.0'
    hooks:
      - id: black
        language_version: python3
        # Use project's Black configuration from pyproject.toml
        files: \.(py)$

  # Flake8 - Python linter (more lenient for this project)
  - repo: https://github.com/pycqa/flake8
    rev: 7.3.0
    hooks:
      - id: flake8
        args: [
          --max-line-length=88,
          "--extend-ignore=E203,W503,F401,F841,E722,E711,E712,E713,E714,F811"
        ]
        files: \.(py)$

  # Additional useful hooks
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v6.0.0
    hooks:
      - id: trailing-whitespace
      - id: end-of-file-fixer
      - id: check-yaml
      - id: check-added-large-files
      - id: check-merge-conflict

and I would have hoped that it fixes anything that our GH linters would find offensive. Instead, I had to repeatedly manually run black on files that the black linter did not like.

I probably didn't configure my pre-commit exactly the way that MDAnalysis wants it. A sample yaml file that "just works" would be useful.

raulloiscuns and others added 4 commits May 28, 2025 10:50

Change in speed conversion factors

154aa84

Fix unit conversion factors in units.py and update corresponding test…

03f1e46

… cases

Merge branch 'develop' of https://github.com/raulloiscuns/mdanalysis …

295a61a

…into develop

Fix speed unit conversion factors in CHANGELOG

5cfb8ab

github-actions bot reviewed May 28, 2025

View reviewed changes

Update velocity formatting in LAMMPS output and adjust reference velo…

a4e32de

…cities for test consistency

orbeckst requested changes Jun 11, 2025

View reviewed changes

orbeckst self-assigned this Jun 11, 2025

orbeckst added the help wanted label Jul 8, 2025

Merge branch 'develop' into develop

8f0ea30

orbeckst mentioned this pull request Aug 22, 2025

Bug in conversion units of speed #5051

Closed

orbeckst added 6 commits August 21, 2025 18:02

Merge branch 'develop' into develop

7174893

Update package/CHANGELOG

6cffd24

Merge branch 'develop' into develop

5632214

updated CHANGELOG

76fdc97

reverted formatting of v in LAMMPS DATAWriter + flake8 fixes

0316137

add A/ns to speed units

5483ebc

- updated table of units - added tests

orbeckst mentioned this pull request Oct 14, 2025

Release v2.10 #5124

Closed

blackified MDAnalysis.units

ea133c9

orbeckst reviewed Oct 14, 2025

View reviewed changes

orbeckst removed the help wanted label Oct 14, 2025

orbeckst added this to the Release 2.10.0 milestone Oct 14, 2025

blackened test_units.py

1f9cd87

IAlibay reviewed Oct 14, 2025

View reviewed changes

move CHANGELOG entries for fixes to newest first order

01251e4

IAlibay requested changes Oct 14, 2025

View reviewed changes

more speed-unit enhancements

0928826

- add tests for unit conversion with unicode characters - add additional representations Å/us, Å/µs, Å/fs, Å/ms

orbeckst force-pushed the develop branch from de6d0b0 to 0928826 Compare October 14, 2025 01:36

orbeckst mentioned this pull request Oct 14, 2025

missing test for LAMMPS DumpReader for ValueError when missing coordinates #5127

Open

orbeckst reviewed Oct 14, 2025

View reviewed changes

IAlibay approved these changes Oct 14, 2025

View reviewed changes

orbeckst added 2 commits October 14, 2025 09:12

set output precision in LAMMPS DATAWriter to .10f for x and v

aea8cf9

- match precision in our test files - required for tests to pass when round-tripping velocities - updated CHANGELOG

blackened utils/test_units.py

e9f24c2

orbeckst force-pushed the develop branch from 24f13eb to e9f24c2 Compare October 14, 2025 16:33

orbeckst enabled auto-merge (squash) October 14, 2025 16:59

orbeckst approved these changes Oct 14, 2025

View reviewed changes

orbeckst merged commit 03eef45 into MDAnalysis:develop Oct 14, 2025
16 of 24 checks passed

Conversation

raulloiscuns commented May 28, 2025 • edited 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 Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orbeckst commented Jun 26, 2025

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IAlibay 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!

orbeckst commented Oct 14, 2025

Uh oh!

IAlibay left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

orbeckst commented Oct 14, 2025

Uh oh!

Uh oh!

orbeckst commented Oct 14, 2025

Uh oh!

orbeckst commented Oct 14, 2025

Uh oh!

RMeli commented Oct 15, 2025

Uh oh!

orbeckst commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

raulloiscuns commented May 28, 2025 •

edited

Loading

codecov bot commented Jun 5, 2025 •

edited

Loading