Repository navigation
Caching of some frame-static attributes - #7949
Conversation
|
Hi there @eteq 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
This is necessary because at any time the user might add new representation or differential classes. Other pacakges (e.g. Sunpy) use this functionality in the wild so it must be supported. This change uses a hash of the rep/diff class dictionaries to detect when a change has occurred.
33dbe71 to
05f245c
Compare
|
At @astrofrog's suggestion during the Astropy telecon today, this is now a stand-alone PR with just the first part (hence the strikethrough in the description above). I may still try to get the latter parts to work, but better to review piecemeal than try to get it all in at the last minute. Note that this does now handle |
68643b5 to
c1db5bb
Compare
|
Note: I tested sunpy against this PR and it works fine. Although the only reason that's true is that a regression test caught an earlier error... a regression test from ~3 yrs ago that @Cadair's usage in Sunpy prompted. Finally the work on regression tests is paying off for @Cadair the Coordinates Canary in a Coal mine! 😉 |
eteq
changed the title
|
Alright I think this may be good to go now - should be ready for review since the main tests are passing now. One implementation note: the validity of the cache depends on it being invalidated when the |
|
Oh and if @StuartLittlefair or @Juanlu001 happen to be available in the next couple days, it might be nice to get an independent pair of eyes on this... (@adrn, @mhvk, and I have been thinking about some of this together for a while now and I'm a bit worried about the groupthink effect) |
adrn
left a comment
adrn
left a comment
There was a problem hiding this comment.
I think this looks ok. A few very minor comments, but otherwise just acknowledge my weariness about using global and I'll be happy with this :)
Co-Authored-By: eteq <[email protected]>
Co-Authored-By: eteq <[email protected]>
|
OK sounds good! The build failures are because of Numpy 1.15.3 issues, so merging. |
2 participants
This is an incomplete implementation of a potential hot-spot in the
SkyCoordinitializer. Specifically, the frequent accessing of certain frame-specific attributes that really don't need to be recomputed a lot (see #7030 for some related disucssion).The first two commit seems to mostly work, except for something I don't fully understand (but can probably figure out how to work around) with
SkyOffsetFrames. Combined they seem to give a 50% speedup in certainSkyCoordinitialization cases.However, the last one is more mysterious: that one caches therepresentation_component_namesat a frame-level. I thought this is supposed to be frame-constant for any particular representation (and that is how the cache is implemented), but for some reason it is causing failures all over, I think at least in part due to either the differentials or some of the related changes in representations. @adrn or @mhvk, do you have any idea what might be going on here? To see the failure in question you can grab this branch and dopython setup.py test -P coordinates- you get a bunch of similar failures that don't seem to have much obvious in common except that they are related to missing names of representation components...