Skip to content

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

Open
GardevoirX wants to merge 1 commit intometatensor:mainfrom
GardevoirX:conversion-bug
Open

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

Conversation

@GardevoirX
Copy link
Contributor

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
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.

1 participant