Units-quantity interaction simplification #1558
Conversation
|
To help review, I rebased. Also removed the |
astropy/units/tests/test_quantity.py
Outdated
There was a problem hiding this comment.
It's important that this remains "is". Base units are identified by their identity, not their string. For example, when a base unit travels through a pickle, it must come back as the same thing on the other side so that (u.m / u.s).is_equivalent_to(u.m / u.s) holds true. Whatever solution we come up with here needs to hold that property.
There was a problem hiding this comment.
@mdboom - I thought is really means it refers to the same object, which is not working any more (and maybe should not work after pickling?). I just checked what other options do and q1.unit.is_equivalent(q2.unit) essentially checks whether they have the same physical type, while q1.unit == q2.unit checks that q1.unit.to(q2.unit, 1.) == 1. In that sense, I think == should be quite safe, but just to be complete I also added an
assert q1.unit.is_equivalent(q2.unit))
There was a problem hiding this comment.
Sorry -- my mistake. I thought this was the test for units, not quantities. We actually do need to ensure that a unit sent over the pickle wire is the same (not just ==). Because units are compared on their identity, not just their name -- i.e. it needs to be possible to define units with the same name as distinct in different namespaces. But I don't think it matters for quantities.
|
@mdboom - also updated the unit-quantity simplification. Should be fine now. |
|
@mdboom - feel free to merge if this looks good to you - I'll let you do a final review. |
…ication Units-quantity interaction simplification
4 participants
At present, when I do, e.g.,
10. * u.m, this leadsUnit.__mul__to returnQuantity(10., u.m). This is fine for this specific case, but becomes problematic if the value is, e.g., aMaskedArray, as then the mask is silently dropped. This PR implements a different logic: if the item being multiplied with is not consistent with being a Unit, the unit converts itself to a Quantity with value of 1, and retries the multiplication, i.e., do10. * Quantity(1, u.m)in the example above. Thus, one avoids presuming the result of something that should really be left toQuantity.__mul__. Happily, it leads to a net decrease in the number of lines of code. (But also triggers one test failure, which I markedxfailas it is not directly a consequence of this PR; see #1557)p.s. Slightly running ahead of the upcoming PR for
MaskedQuantity, I do not want to haveQuantity(masked_array, unit)return aMaskedQuantity, since that is contrary to the behaviour of bothnp.arrayandColumn, both of which simply strip the mask.EDIT -- this PR includes #1554, which I kept separate as it should be (even more) uncontroversial
EDIT2 -- #1554 and #1557 have been merged.