Cache frame representation info; doubles speed of coordinate generation. - #5952

Your changes broke the tests.

So it does.

Do you have any recommendations of where to invalidate the cache when a new subclass of a frame is created? I thought that would be handled by the Metaclass.

       class FakeICRS(ICRS):
            frame_specific_representation_info = {
                'spherical': {'names': ('ra', 'dec', 'distance'),
                              'units': (None, None, None)},
                'unitspherical': {'names': ('ra', 'dec'),
                                  'units': (None, None)}
            }
    
            frame_specific_representation_info = {
                'spherical': [RepresentationMapping('lon', 'ra', u.hourangle),
                              RepresentationMapping('lat', 'dec', None),
                              RepresentationMapping('distance', 'distance')]  # should fall back to default of None unit
            }
            frame_specific_representation_info['unitspherical'] = \
                frame_specific_representation_info['spherical']

Maybe @eteq knows?

I don't really understand what it is used for and why. (All I know is that a complex nested loop is called 22 times every time you want to represent the most basic element of positional astronomy.)

Moving it to somewhere else would require a deeper understanding of the code, its structure, and its history and customs than I have time to acquire at the moment.

I have no objection to someone else providing a better (and working) speed-up.

@dmopalmer - fair enough; since I also do not understand all that well why this code is needed or called so often, maybe best is to ping someone else... @eteq: Could you have a look?

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please close this and open an issue instead to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If you believe I commented on this issue incorrectly, please report this here.

Ideally, this is still tackled, but in pieces.

⏰ Time's up! ⏰

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

I'm reopening this but milestoning it for 3.1 - slated for that is a major re-write of the SkyCoord initializer in the name of performance. It may be this becomes irrelevant at that time, in which case we should close it.

Hi there @dmopalmer 👋 - 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 😃.

I noticed the following issue with this pull request:

• Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

If there are any issues with this message, please report them here.

To answer the underlying question of why it's called so often: getting the representation info is needed to do all kinds of things like asking "what are the names of the attributes used on this frame". That's done in the SkyCoord initializer because it does a lot of "taking apart" an input object and then "putting it back together again". One of the goals in the v3.1 optimization would be to undo that, possibly making this cache less/unnecessary.

But more generally, the problem with caching this in the manner this PR does is that in theory it can change. I'm not sure if anyone actually uses this, but it might happen in some intermediate manner during class creation? My suspicion is that this is the origin of the failure. But some slightly cleverer caching should still be possible...

See #6941 for a PR that improves item access by a factor 50. There is definitely low-hanging fruit that should get picked before we add more layers of caching (though this particular layer does still seem reasonable).

@dmopalmer - while this issue sat for a long while, in #7949 I finally cracked the case and got this caching working. While it was not directly based on this PR (due to a lot of changes required for velocities that made it hard to do so), it was nonetheless inspired by your attempt here. So I just now pushed up a "no-op" merge commit to this branch, which will enable us to merge this PR to record the commits in the git history so that we can be sure you get credit for the effort here that led to #7949 .

Thanks for the work here!