Skip to content

Fix a bug in converting units for List[System]#174

Open
GardevoirX wants to merge 2 commits intometatensor:mainfrom
GardevoirX:conversion-bug
Open

Fix a bug in converting units for List[System]#174
GardevoirX wants to merge 2 commits intometatensor:mainfrom
GardevoirX:conversion-bug

Conversation

@GardevoirX
Copy link
Contributor

@GardevoirX GardevoirX commented Mar 5, 2026

In _convert_systems_units, we calculate the unit conversion factor, conversion, for length, and then we enter a for-loop, to apply the conversion to systems:

if model_length_unit == "" or system_length_unit == "":
# no conversion for positions/cell/NL
conversion = 1.0
else:
conversion = unit_conversion_factor(
quantity="length",
from_unit=system_length_unit,
to_unit=model_length_unit,
)
new_systems: List[System] = []
for system in systems:
new_system = System(
types=system.types,
positions=conversion * system.positions,
cell=conversion * system.cell,
pbc=system.pbc,

However, when there are additional inputs attached to a system, to convert them to our requested unit, we calculate the conversion factor, and also store it in conversion:

if requested.quantity != "" and unit is not None:
conversion = unit_conversion_factor(
quantity=requested.quantity,
from_unit=unit,
to_unit=requested.unit,
)
else:
conversion = 1.0

This covers the original conversion factor, and in the next round of for-loop, we are using a wrong length conversion factor.

This is fixed by renaming the conversion factor used for length to length_conversion, but I am not sure if it's the best solution, and if we need to add some tests to prevent this from happening again in future

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

Reviewer checklist

  • CHANGELOG updated with public API or any other important changes?

@GardevoirX GardevoirX requested review from HaoZeke and Luthaf March 5, 2026 16:45
Copy link
Member

@HaoZeke HaoZeke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @GardevoirX. The fix is correct and minimal. One suggestion: it might be worth adding a regression test to prevent this from happening again. Something that tests:

  • Multiple systems with unit conversion
  • Additional inputs that also need unit conversion

Another thing is that in a follow up we can maybe hoist these into external functions but for now I think a test would be enouh.

@GardevoirX GardevoirX requested a review from HaoZeke March 9, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants