Skip to content

Fix Quantity * Quantity and / Quantity on compound ratio units - #5478

Merged
peastman merged 3 commits into
openmm:masterfrom
barlowa124:fix-quantity-compound-ratio-mul-div
Oct 5, 2026
Merged

peastman merged 3 commits into
openmm:masterfrom
barlowa124:fix-quantity-compound-ratio-mul-div

Conversation

@barlowa124

Copy link
Copy Markdown
Contributor

Summary

Fixes #5445.

Quantity.__mul__ delegated Quantity * Quantity to (Quantity * scalar) * unit, and __truediv__ mirrored it. When self's unit is a dimensionless compound ratio such as nanometer/angstrom, the scalar half collapses the unit to dimensionless and returns a bare value, so the following unit operation sees only the other operand's unit:

a = 3.0 * (nanometer / angstrom)
a * a   # was "9.0 nm/A", now 900.0, matching a ** 2
a / a   # was 0.1, now 1.0
a / b   # was 15.0 for b = 2.0 A/nm, now 150.0

The fix multiplies or divides the raw values and combines both units in a single Quantity before reduce_unit. Operating on raw values also preserves the previous float association for ordinary units, so 1.3 m / 75.2 cm still produces the bit-identical result TestUnits.testQuantityMaths asserts.

Testing

Added testCompoundRatioMultiplyDivide to TestUnits.py covering multiply and divide on ratio units (including self-similar, inverse, and mixed-scale ratios). A non-dimensionless sanity check confirms ordinary products are unaffected. All 55 tests in TestUnits.py pass.

Checklist

  • Ran targeted tests locally (TestUnits suite, 55 passed)
  • Is ready for review and does not include unfinished work

Generated by Devin

Quantity.__mul__ delegated quantity*quantity to (quantity*scalar)*unit
and __truediv__ mirrored it. When self's unit is a dimensionless
compound ratio such as nanometer/angstrom, the scalar operation
collapses it to a bare value first, so the later unit operation
reattached only the other quantity's unit. Products dropped a unit
factor (3.0 nm/A squared gave "9.0 nm/A" instead of 900.0) and
quotients inherited the wrong unit entirely (a/a gave 0.1, a/b gave
15.0 instead of 150.0).

Multiply/divide the raw values and combine the units in one Quantity,
then reduce_unit as before. This preserves the existing float
association for ordinary units while fixing the ratio-unit collapse.

Fixes openmm#5445.
@peastman

peastman commented Oct 3, 2026

Copy link
Copy Markdown
Member

Looks good. With the changes from #5477, however, your test case no longer tests what you intend since 3.0 * (u.nanometer / u.angstrom) now evaluates to just a float, not a Quantity. You can change it to u.Quantity(3.0, u.nanometer / u.angstrom) to get a Quantity with dimensionless units.

Since openmm#5477, scalar * dimensionless-unit expressions evaluate to plain
floats, so 3.0 * (u.nanometer / u.angstrom) no longer produces a
Quantity and the test stopped exercising Quantity arithmetic.  Build
the test values with u.Quantity() directly.
@barlowa124

Copy link
Copy Markdown
Contributor Author

Updated in 0c19ede. The test now builds the values with u.Quantity() directly, so a * a still goes through Quantity.__mul__.

@peastman

peastman commented Oct 5, 2026

Copy link
Copy Markdown
Member

Tests are failing:

>           return Quantity(self._value / other._value,
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^
                            self.unit / other.unit).reduce_unit(self.unit)
E           TypeError: unsupported operand type(s) for /: 'list' and 'float'

The raw value*value and value/value paths raise TypeError when either
Quantity wraps a plain list or tuple (e.g. Vec3 values). Fall back to
_scale_sequence in that case, matching _change_units_with_factor.

Generated with [Devin](https://devin.ai)
@barlowa124

Copy link
Copy Markdown
Contributor Author

Fixed in 23c98db. The raw value * value and value / value paths
raised TypeError for Quantities wrapping lists or tuples. Both
branches now fall back to _scale_sequence, the helper
_change_units_with_factor uses for the same case.

TestExpandedEnsembleSampler::testReporter, TestUnits, and
TestAPIUnits pass locally against this change.

@peastman
peastman merged commit d319a3e into openmm:master Oct 5, 2026
15 of 16 checks passed
@peastman

peastman commented Oct 5, 2026

Copy link
Copy Markdown
Member

Thanks!

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.

Quantity * Quantity` does not square a compound ratio unit

2 participants