Repository navigation
Make hashes of units a bit more logical, guarding against unlikely cases - #20224
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
This lets the hash of a composite unit depend on the hashes of the bases, instead of just their names, and that is short-circuits the case of a single base with scale=1 and power=1. To make this work, the irredicible unit gets a new hash (arguably an improvement in itself).
8d9a1ca to
781217d
Compare
| return hash((self.scale, *[x.name for x in self.bases], *map(str, self.powers))) | ||
| # if we're just a wrapper around another unit, let our hash be the same | ||
| # as that unit's. | ||
| if self.scale == 1 and len(self.powers) == 1 and self.powers[0] == 1: |
There was a problem hiding this comment.
This ensures that hash((u.m**2)**.5) continues to equal hash(u.m) (before, both expanded to hash((1.0, "m", "1")).
There was a problem hiding this comment.
Note: I considered changing CompositeUnit.__init__ to __new__ so that it can return the original unit in this case, but it seemed more involved and risky, and more of an API change.
There was a problem hiding this comment.
I agree. I think we can make a classmethod for strange constructors, e.g. CompositeUnit.make, but standard constructors should return standard objects (isinstance(CompositeUnit(), CompositeUnit)).
There was a problem hiding this comment.
Ah, I hadn't thought about the constructor argument; agreed we should only do it for a strong case -- which this is not.
| self.__getstate__(), | ||
| ) | ||
|
|
||
| @cached_property |
There was a problem hiding this comment.
This used to just inherit, and thus be hash((1.0, self.name, "1")). But that would now lead to infinite recursion. The new choice makes it more similar to NamedUnit, which does hash((self.name, self.represents)). Possibly hash((self.name, None)) is more logical, but it seemed nice to be a bit more unique.
|
@mhvk does changing hashes change any code in a big way, e.g. serializations? Just thinking about the scope of this change. If it's actually far-reaching then we should take a sec to standardize the hash layout so all unit classes can have the same recipe. E.g. what's the maximum number of slots we need? For |
|
@nstarman - in the end, a hash is just an integer, calculate in some way (e.g., |
Good. Then I agree that I think we're relatively free to iterate on the hashes even within dot releases. |
|
Those are unrelated, thankfully: #20164 and the |
|
OK, I'll be a bit blunt and merge so I can rebase #20107 and open that back up for review; @eerovaher - I'll be happy to do a follow-up if you have suggestions. |
This PR aims to make a bit better effort to make hashes of units logical (and is needed for #20107, where converters are cached).
The basic idea is to let the hash of a composite unit depend on the hashes of the bases, instead of just their names, and
to short-circuit the case of a single base with scale=1 and power=1.
To make this work, the
IrredicibleUnitgets a new hash (arguably an improvement in itself).p.s. This does not aim to address the difference with unit equality and hash equality.
EDIT: I see no point in backporting this.