Repository navigation
Fix units returned by multiply, divide/floor_divide, and square - #61
Open
Omair-Asdaq wants to merge 6 commits into
Open
Omair-Asdaq wants to merge 6 commits into
Omair-Asdaq wants to merge 6 commits into
Conversation
Omair-Asdaq
marked this pull request as ready for review
October 17, 2026 18:47
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
Omair-Asdaq
marked this pull request as draft
October 19, 2026 19:28
floor_divide floored the raw magnitudes of its operands but labelled the
result with the quotient units x1.units / x2.units. The quotient unit
carries a scale factor that the raw floor division ignored, so whenever
the two units differed by a scale factor the answer was wrong by
exactly that factor:
floor_divide(2 m, 3 cm) -> 0.0 meter / centimeter (should be 66)
floor_divide(1 m, 1 mm) -> 1.0 meter / millimeter (should be 999)
floor_divide(2 min, 3 s) -> 0.0 minute / second (should be 39)
The result was also not invariant under re-expression of the operands:
the same physical ratio gave 0, 66 or 666 depending on the units used.
Floor division of two quantities is a count -- the number of whole
right-hand-side units that fit in one left-hand-side unit -- so the
operands have to be brought onto a common scale before flooring, and the
result is dimensionless. That is what pint's Quantity.__floordiv__ and
this module's own // operator do, and it is what divmod returns as its
quotient.
Convert the right-hand side into the left-hand side's units first, which
also raises DimensionalityError for dimensionally incompatible units,
and return a dimensionless count.
The test that asserted floor_divide(6 m, 4 s) == 1.0 m/s pinned the
divergent behaviour and is replaced by one that expects the error. The
scale factor case, an invariance check, and a check that the function
agrees with the // operator are added.
Omair-Asdaq
marked this pull request as ready for review
October 20, 2026 03:09
prod computed the unit exponent as magnitude.shape[axis], which raises TypeError when axis is a tuple of axes, because a tuple cannot index the shape. The array API allows a tuple of axes, and the magnitude itself was already computed correctly, so only the unit was wrong. Reduce over each axis and multiply the sizes together to get the number of elements reduced, which is the exponent the unit has to be raised to. For a single axis and for axis=None the result is unchanged.
The xp-tests task cloned array-api-tests from master every run, so the suite moved under the pinned array-api-strict. Tests that the pinned backend does not implement yet -- finfo/iinfo with an array argument, added in array-api-tests#358 -- then failed, and CI had been red for every pull request since. Pin the checkout to the last commit that the pinned array-api-strict passes, so that a failure in this job means a real regression.
Omair-Asdaq
marked this pull request as draft
October 20, 2026 03:33
Omair-Asdaq
marked this pull request as ready for review
October 20, 2026 14:31
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three elementwise functions build their result's units incorrectly. I noticed it while checking that the function forms agree with the operator forms.
multiply— converted magnitude, unconverted unitsmultiplycomputesunits = x1.units * x2.unitsbut feeds the second operand throughx2.m_as(x1.units). The magnitude ends up expressed inx1.unitswhile the units are labelled as the product:multiply(1 m, 1 mm)→1e-06 m², should be1e-03 m²(off by the conversion factor)multiply(1 m, 1 s)→DimensionalityError, should givem·sUsing the raw magnitude together with the product of the two units agrees with the
*operator, which was already correct.divide— result carried the dividend's unitsdividecame out of the generic elementwise loop, which setsunits = x1.unitsand convertsx2intox1.units:divide(2 m, 1 m)→2 m, should be dimensionless2divide(6 m, 3 s)→DimensionalityError, should be2 m/sIt now has its own implementation using
x1.units / x2.units, mirroring the waymultiplyis already special-cased.square— units not squaredsquarecame from the generic single-array loop, which returns the input units, sosquare(3 m)had unitsm.sqrtalready special-cases this (x.units**0.5);squarenow returnsx.units**2.floor_divide— floored on incompatible scalesfloor_dividehas its own implementation usingx1.units / x2.units, mirroring the waymultiplyis already special-cased.Floor division of two quantities is a count — the number of whole right-hand-side units that fit in one left-hand-side unit — so the operands have to be brought onto a common scale before flooring, and the result is dimensionless. This is what
pint'sQuantity.__floordiv__does, what this module's own//operator does, and whatdivmodreturns as its quotient. The operands are therefore converted onto a common scale first, which also raisesDimensionalityErrorfor dimensionally incompatible units.prod— tuple axisprodcomputed the unit exponent asmagnitude.shape[axis], which raisesTypeErrorwhenaxisis a tuple of axes, because a tuple cannot index the shape. The array API allows a tuple of axes, and the magnitude was already computed correctly, so only the unit was wrong. The exponent is now the product of the sizes of the reduced axes.Tests
One regression test class per fix, asserting both the units and the numeric magnitude (normalised through
m_as). All of them fail on the currentmasterand pass with these changes. Full suite: 64 passed, 4 xfailed — the same 4 that are xfailed today.I ran the suite against the
array-api-strict==2.2pinned inpixi.lock; a newer release rejects a list forshapeand makes six unrelated tests fail.CI
The
xp-teststask clonedarray-api-testsfrommasteron every run, so the suite moved under the pinnedarray-api-strict. Tests that the pinned backend does not implement yet —finfo/iinfowith an array argument, added in array-api-tests#358 — then failed, and CI had been red for every pull request since. The checkout is now pinned to the last commit the pinnedarray-api-strictpasses, so a failure in that job means a real regression.Two things I noticed but did not change
//operator strips units entirely and raises for dimensionally different operands, because__floordiv__is bound by the genericunitless_binary_namesloop. That is pre-existing and separate, but it means the newfloor_dividecan't agree with//until it's addressed.Unitarithmetic,multiply(1 degC, 1 m)anddivide(1 degC, 2 s)returndegC·manddegC/swhere they previously raised. That is consistent with how the module already handles operation-defined units (prod,var,sqrt), and withpint's own array-function path, but it is a behaviour change worth a look.