Repository navigation
Cache/pre-compute representation_info #2989
Description
I was spending a little time profiling parts of astropy.coordinates and I was surprised to see that get_representation_info gets called 12 times when doing FK5(3.2 * u.deg, 2.2 * u.deg):
%prun FK5(3.2 * u.deg, 2.2 * u.deg)
ncalls tottime percall cumtime percall filename:lineno(function)
12 0.001 0.000 0.001 0.000 baseframe.py:534(_get_representation_info)
316 0.000 0.000 0.000 0.000 _collections_abc.py:462(__iter__)
322 0.000 0.000 0.000 0.000 __init__.py:83(__iter__)
6 0.000 0.000 0.000 0.000 quantity.py:168(__new__)
207 0.000 0.000 0.000 0.000 {built-in method isinstance}
...
In fact, this takes up a significant fraction of the time (80% of the time of initializing a new frame!). So a few questions:
- Why does it get called 12 times?
- Can we simply cache this since it is a class method? (and will in future be a property that depends only on the class)
- Can we even pre-compute this at import time?
Activity
embray commentedon Sep 29, 2014
Definitely seems odd to have to recompute every time. Maybe instead there should be some little internal representation "database" (where by "database" I mean nothing more than the data structure this is already returning). It could have a method to add new representations to that database (which would happen automatically in
MetaBaseRepresentation. So that rather than the metaclass just updating theREPRESENTATION_CLASSESdict it would update the full datastructure. so that this doesn't have to be recomputed every time.eteq commentedon Sep 29, 2014
That seems like a lot, but not necessarily surprising, because I think it's used a lot in different layers of the code to figure out where to store various variables (and they don't pass these names, so it's up to this property to provide them).
Almost certainly, yes. It's definitely safe to cache it in the property - we could just change the current
representation_infoproperty to:If you look in the code for
_get_representation_info, you'll see a comment at the top that says it should be moved to the property once it no longer needs to be a class method for the deprecatedunitsusage that acts like v0.3.It could be left as a classmethod that caches it the first time it's used, but I'm a bit worried this has unintended consequnces I haven't recognized. Depends on how much better that will be than caching on a per-instance basis, I guess.
Probably, but I think it's better not to, so that code importing astropy but not needing coordinates doesn't have to do this.
astrofrog commentedon Sep 30, 2014
Can we use
@lazypropertyto do the caching? In any case, I'm +1 on moving the code to be a property now.embray commentedon Sep 30, 2014
Using a property has nothing to do with caching per se.
@lazypropertydoes though. But that doesn't seem like a good idea either because if new representations are imported from elsewhere it invalidates the cached value.That's why I was recommending updating
MetaBaseRepresentationso that the "registry" of representations is the data structure currently returned by_get_representation_info, rather than merely thatREPRESENTATION_CLASSESdict.eteq commentedon Sep 30, 2014
I don't quite understand how that works, @embray: what
_get_representation_infoyields is different for every frame. Basically, what_get_representation_infodoes is look through all the representations and ask "are the names and units of this representation inframe_specific_representation_info? If yes, use that, if no, use the defaults". So it has to know about the specific frame it's getting called on.Thinking about this lead me to realize an issue with
@lazypropertythough: this does change if a new representation gets added (as @embray said). So that's why it was a classmethod in the first place: it has to stay up-to-date whenever a new representation class gets create, which could conceivably be at any time. And it also has to know theframe_specific_representation_info. The latter could be updated, but that's sort of an oversight rather than intended behavior (it's a read-only property, but you can change the values in the dictionary).So if we want to say that
frame_specific_representation_infois immutible and set in stone when the class is defined, we could create a class-level property (using the trick with metaclasses @embray mentioned somewhere else), which acts like@lazyproperty, but also keeps a hash ofREPRESENTATION_CLASSESand re-calculates everything only if it changes. If that sounds good to both of you, @embray and @astrofrog, I can take a shot at implementing it.eteq commentedon Sep 30, 2014
Thinking about it just after typing that, @embray, did you just mean that we could simplify
_get_representation_info's logic instead of eliminating it? I was assuming the latter, but when I think about it, your suggestion could be used in conjunction with what I said above.embray commentedon Sep 30, 2014
I think so. I had assumed
frame_specific_representation_infowould never change (or at least that it shouldn't).astrofrog commentedon Sep 30, 2014
@eteq - I like your idea but we could also make it that defining a new representation means it auto-registers and triggers the representation info class attributes to be recomputed, rather than using hashes?
eteq commentedon Oct 20, 2014
@astrofrog - I suppose that would work too. The main downside I see is that it would slow down imports - with a lazy-type property, it won't do the calculations until the first time you access it, while if it's at new class creation, it has to be re-calculated for every new class. But that's probably pretty small overhead either way.
adrn commentedon Dec 3, 2018
This was partially implemented by @eteq. If we think there is more to gain from smarter caching, let's create a new issue to discuss!