Implement ~20x faster SkyCoord getitem using cached template - #6896

Hi there @taldcroft 👋 - 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 see this is a work in progress pull request. I'll report back on the checks once the PR is ready for review.

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

There's a problem when changing representations, but I have an idea how to fix.

@mhvk @astrofrog - passing tests locally now.

Ping @mhvk @eteq @astrofrog. Anyone?

This looks really interesting! So just to be clear, even with the deepcopy you are getting a 20x speedup? (if so - wow!). Also I guess the deepcopy does return a different SkyCoord for each item?

So just to be clear, even with the deepcopy you are getting a 20x speedup? (if so - wow!). Also I guess the deepcopy does return a different SkyCoord for each item?

Yes and yes.

Though I see now the uniqueness of returned values is not tested. Will do that, but in development I extensively tested by doing list(sky_coords) and seeing that they were all different.

BTW, without the uniqueness requirement one can have a fastiter() method that is something like a few hundred times faster since you avoid the deepcopy. @mhvk was opposed to "polluting" the API this way, but it may still be worth discussing.

Ok and just to be clear, if we speed up SkyCoord.__init__ by 20x in future, can we remove this and not break backward-compatibility? (I think yes?)

@astrofrog, @taldcroft - I'm trying to put together a hack that just skips __init__ for cases like these, so we have something to compare against. But it is near the end of term here, and I have very little time. A week from now things should have quieted down a bit.

if we speed up SkyCoord.init by 20x in future, can we remove this and not break backward-compatibility?

Yes, this PR introduces no change in public API. Obviously it adds a private attribute and method, but these can be removed in the future with no notice.

I now tried cleaning up getting items, making sure one doesn't pass through deadly slow __init__ - I get a factor of nearly 50 from that! See #6941. There's definitely more to be done, but even now I think it proves we don't have to add extra hacks to try to paper over not-quite-smart code (quite a bit of it my own...)

@mhvk - can you clarify : if you combine this with #6940 / #6941, do you get an additional speedup, or do thus essentially replace this? (my quick once-over suggests it does, but should be checked to determine whether this is worth keeping at least temporarily).

Assuming that is the case, I think we hold onto this one in case problems appear in #6940 that prevents merging in time for 3.0, as I think this is a good thing to have if it's not replaced by the other two.

Personally, I consider all this work unfair right now though. What's the point of having a performance-focused release in 3.1 if we do it all now? 😉

It looks like this will be superceded by #6940 / #6941 if they work out. Great work @mhvk!

Timings on my mac laptop show that this PR still increases getitem speed by a factor of more than 11 after merging #6940 (which itself improved speed by a little less than a factor of two). Unless #6941 gets merged then the coordinates team should still consider this one.

Astropy 2.0

In [2]: sc = SkyCoord(np.linspace(0, 10, 100), np.linspace(0, 10, 100), unit='deg')

In [3]: timeit list(sc)
302 ms ± 9.44 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

current master (with #6940 merged)

In [8]: timeit list(sc)
10 loops, best of 3: 176 ms per loop

With this PR (rebased on current master with #6940)

In [3]: timeit list(sc)
100 loops, best of 3: 15 ms per loop

@

Went ahead and merge #6941, so this one won't go in to 3.0.

More generally, I think we are agreed that we don't want to implement too much caching until we've taken a first crack at cleaning up the SkyCoord initializer and deciding where the caching should go (also see #7029). With that in mind, @taldcroft, do you think we should leave this open for consideration in that process, or do you think this should be closed?