Repository navigation
Performance improvements for astropy.units - #2544
Conversation
- For compose, the set of destination units are prefiltered to only include units that have bases in common with "self". This cuts down on the search space considerably in most cases. - Inline some delegation functions. - Replace 2 isinstance calls with 1 - Cache the hash of composite units, which can be expensive to compute.
|
So the reason why it's safe to cache the hash (say that 3 times fast...) is because |
|
Yes -- this works because they are immutable. I don't think there'd be any benefit to having a global dictionary of hashes as you describe, since you'd have to compute the hash to get the hash anyway. |
|
Oh, duh, good point. I guess dictionaries aren't just magic, after all... Also, note that I canceled the travis build on 3143b82 because our queue is really jammed up (and it's just a changelog updating commit) EDIT: now that I think about it, it probably would have made more sense to kill the earlier one... but it shouldn't matter here because the changelog change is trivial |
There was a problem hiding this comment.
cgs and si could just as well use @lazyproperty rather than saving it explicitly off to an attribute (ultimately the same thing just gets rid of a little boilerplate).
There was a problem hiding this comment.
Good point. I had forgotten about lazyproperty.
There was a problem hiding this comment.
@mdboom - I'm still not completely sure how this is supposed to work, but think you may need to deal with the tunit is None case as well. Likely, the only equivalency to use this will remain dimensionless_angles, and for this case, if, e.g., other has radian as a base, while self does not, should one not conclude this is a fair base to try?
I guess without further looking into this, I would suggest adding a test case:
u.Unit("rad/s").compose(units=(u.day,), equivalencies=u.dimensionless_angles())
but looking at the current master, this fails, so perhaps this is outside of the scope of this PR (if you think so, I'll make a separate issue).
|
Alright, in that case I'll just go ahead and merge this, as the tests are all happy. |
Performance improvements for astropy.units
|
|
include units that have bases in common with "self". This cuts down
on the search space considerably in most cases.
This is partially in response to #2479, though it probably makes sense to also include #2542 as well (though this and #2542 are completely independent).