Skycoord baseframe method speedup by mhvk · Pull Request #6941 · astropy/astropy

mhvk · 2017-12-07T18:47:54Z

This builds on #5598 and #6940 to speed up item access in SkyCoord by a factor of almost 50. The main change is for _apply to create a new instance itself, and fill it as appropriate, i.e., to avoid the extremely long and slow parsing and error checking in the __init__ methods of BaseRepresentation, BaseFrame, and SkyCoordinate. To me, this is a rather striking lesson in the dangers of trying to be overly generous in what to accept; this would not have been a problem if the various ways of initialization all had their own class methods.

What I am still very unsure about, however, is whether the current approach, of just doing __new__ in _apply and fill the object, is the best one. It does mean that if people do something important in __init__ in a subclass, that now gets skipped. Indeed, you'll see I already have to copy over some cached information from self... Furthermore, I had to make BaseFrame.realize_frame go through a new _replicate method to ensure I wouldn't get hit by a regression test for SunPy that changes the default representation in __init__; #6236. While this is much cleaner, it does make one wonder about other possible regressions.

As an alternative to just replicating the actual stuff __init__ does, one could imagine passing on an argument to __init__ that states all arguments are OK and default (say, copy=None) or perhaps making a new class method from_vetted_data or so. Suggestions most welcome!

EDIT: I do think this can and arguably should be done by rewriting __init__ such that it is fast for simple & correct data. In a way, this PR should be seen as showing what is possible, not necessarily as what I would like merged. But we definitely should not merge any speed-ups that work by caching before we actually have gone over these classes properly and removed the obvious bottlenecks.

c = SkyCoord(np.arange(30.)*u.deg, np.arange(30.)*u.deg)
%timeit c[1:2]
# 2.0.2: 100 loops, best of 3: 2.94 ms per loop
# this PR: 10000 loops, best of 3: 59.1 µs per loop

astropy-bot · 2017-12-07T18:47:56Z

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

taldcroft · 2017-12-08T11:22:19Z

I will say that caching was quite a thorn in implementing mutability of Time, so in general 👍 on getting away from caching where possible.

astrofrog · 2017-12-14T13:27:50Z

I spent way longer than I had intended thinking about this and looking at the current code. Specifically I was thinking about the idea of from_vetted_data.

I think the key to all this is to make it possible to instantiate SkyCoord from frames with data if there are no positional arguments (to avoid data merging issues). That is, the following should work:

In [8]: coord = SkyCoord([1, 2], [1, 2], unit='deg', frame=frame)

In [9]: SkyCoord(frame=coord.frame[0], copy=False)

If we can get this to work and if we can have a shortcut for when there are no args to detect this, then this could speed things up significantly in SkyCoord's __init__. Now it does mean we need to make sure that item access is fast in frame classes, but we should do that anyway. I think this solution would be cleaner than e.g. from_vetted_data.

mhvk · 2017-12-14T13:39:17Z

@astrofrog - I agree with you that it would be much nicer to go through __init__. Note, though, that in the general case there will also be SkyCoord attributes to set (things that do not belong on the frame, like obstime for ICRS). Hence, my alternative of copy=None, with the suggestion that that is even stronger than copy=False - no checks are done at all (except perhaps a test that there is only one positional argument, and that that is a frame, but no (very slow) checking of attribute names, etc. I do worry this will still be somewhat slower since one has to go through super calls, etc. But it does mean that if someone already has a subclass that implements copy=False but not None, things will in general just work (which is not the case if we introduce a new keyword argument).

Anyway, I don't have the time right now, but I'll try to make an alternative PR that uses __init__, so that we can compare.

Cadair · 2017-12-14T15:14:12Z

I apparently need to carve out some time to look at this in more detail. The following however made me want to comment sooner rather than later:

It does mean that if people do something important in __init__ in a subclass, that now gets skipped.

SunPy does some rather funky and important things in the __init__ of our frames. i.e. this or this.

I also agree wholeheartly with this:

To me, this is a rather striking lesson in the dangers of trying to be overly generous in what to accept

The constructor of SkyCoord is completely bonkers. Also, in writing those __init__s for the SunPy frames I realised how crazy the one for BaseCoordinateFrame is as well.

taldcroft · 2017-12-14T16:17:36Z

The constructor of SkyCoord is completely bonkers.

Well we did have agreement that SkyCoord would be extremely liberal what it accepted, and that's what I coded. 😄

But maybe now we do need to revisit the original design decision? Perhaps a new parameter or a new subclass to heavily restrict the allowed inputs for improved performance?

mhvk · 2017-12-14T16:33:27Z

@Cadair - it don't think it is that bad -- @eteq, @adrn and I discussed this at some length, and we think it can be made more clear what happens (and probably faster at the same time) without too much effort, partially by moving quite a bit of stuff to the frame transformation graph. Also, part of the problem is that we kept adding things without revisiting the structure.

More on topic of this PR: the good part of the normal _apply -- which deals just with shape-changing, slicing, etc., is that it keeps all attributes of the same type - so really there should be no reason to do any hacking on it. (This is not true for transform_to, though, which is why I ensured that those paths still go through __init__; indeed, without doing that, it was the "sunpy test" that broke!).

Cadair · 2017-12-14T18:23:40Z

it don't think it is that bad

😆 yeah it's not. I think the SkyCoord constructor being that flexible with input is a good thing. I think restricting down the lower layers for performance reasons could be justifiable.

it was the "sunpy test" that broke

🤔 makes me wonder if I should write more of them. Maybe a test that uses the __init__ of a frame to convert from 2D to 3D.

mhvk · 2017-12-14T18:30:36Z

@Cadair - further tests are always good, though you guys do sufficient testing with astropy-dev that I think we're effectively already including your tests!

astrofrog · 2017-12-14T22:25:27Z

I think it's fine to have the initializer be very flexible, but maybe it's a question of clearly documenting in what order things happen and having ways of taking shortcuts for performance. That is, we do the fast checks first and only if things don't match do we then move to the next level of slowness, and so on. Then e.g. initializing a SkyCoord with a frame with data and nothing else would be super fast whereas initializing with coordinates as strings and with lots of attributes would be slower.

taldcroft

The changes in SkyCoord._apply look sensible to me. Overall I'm bowing out on this one to current coordinates maintainers / developers.

eteq · 2017-12-22T05:33:40Z

we do the fast checks first and only if things don't match do we then move to the next level of slowness, and so on.

I think there's probably a lot of space in the SkyCoord initialization to improve things just like this - as @taldcroft and @mhvk pointed out, it's getting to be time to re-visit the whole method since that machinery has evolved sort of "organically" (i.e. without a clear plan after the first version with SkyCoord), and 3.1 being a "performance"-oriented release is a great time to re-visit that. It will also probably fix several subtle bugs that I think exist because of the twisted logic (but just don't have clear use cases that have led to issues).

Probably the thing to do is try to do this approach @astrofrog is suggesting first along with the larger clean-up, but then if it becomes clear that even with restructuring we can't eke out more performance, then we try the more "restrictive" option?

(comments on this PR itself next)

eteq

I share @mhvk et al's concern about the bypassing of __init__... The key question is this: do we think we can re-structure the SkyCoord initializer in 3.1 to make it so that this is no longer necessary? If so we may just want to live with the costs in __init__ for 3.0 so that we don't bypass it now but then just bring it back in 3.1...? But if the only folks who care right now are SunPy, and @Cadair is happy with this, then I think this is an acceptable "stopgap".

If this is to go in, though, it definitely needs a changelog entry that points out that __init__ is now getting bypassed in these cases (probably under "API changes"?), as that's a subtle but important API change. This also needs a rebase now that #6940 is in (which this includes as it stands).

eteq · 2017-12-22T05:40:32Z


    representation_component_units = property(get_representation_component_units)

+    def _replicate(self, data, copy=False, **kwargs):


I know this is private, but it might be useful to provide a docstring with parameters here, given how convoluted this logic can get...?

Currently, getting an item or slice causes a new representation object to be created, which does a lot of tests on the inputs, which are not necessary, since the data is prevalidated. By skipping this, we can speed up accessing a single element by a factor of about 4.

Partially by splitting out replication stuff, which needlessly confuses it, and which still has to go through __init__.

mhvk · 2017-12-22T16:10:02Z

@eteq - OK, I rebased, added a docstring to _replicate and wrote a changelog entry. In the end, I think this approach is fine; we should just reconsider when we have re-done the initialization whether it still makes sense.

mhvk · 2017-12-22T20:52:21Z

@eteq - are you happy with the changes?

adrn · 2017-12-22T22:19:24Z

I'm afraid I won't have time to do a thorough review of this today, but I just want to say that conceptually I agree with @eteq here:

do we think we can re-structure the SkyCoord initializer in 3.1 to make it so that this is no longer necessary? If so we may just want to live with the costs in init for 3.0 so that we don't bypass it now but then just bring it back in 3.1...?

But I also agree with the more practical point:

if the only folks who care right now are SunPy, and @Cadair is happy with this, then I think this is an acceptable "stopgap".

So: that's a long way of saying that I'm fine if this goes in, but would also be fine leaving as is and just promising performance improvements with 3.1

eteq · 2017-12-23T03:28:10Z

Alright, I think it's worth the performance improvement, so will merge, but I'm going to make a new issue and milestone it for 3.1 to make a final decision on "__init__ or not"

mhvk added coordinates no-changelog-entry-needed Performance labels Dec 7, 2017

mhvk added this to the v3.0.0 milestone Dec 7, 2017

mhvk requested review from adrn, eteq and taldcroft December 7, 2017 18:47

This was referenced Dec 7, 2017

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

Closed

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

Merged

Change BaseFrame.__setattr__ to be fast for private attributes. #6940

Merged

taldcroft reviewed Dec 18, 2017

View reviewed changes

eteq requested changes Dec 22, 2017

View reviewed changes

eteq removed the no-changelog-entry-needed label Dec 22, 2017

mhvk added 4 commits December 22, 2017 10:45

Make BaseFrame._apply much faster.

3156863

Partially by splitting out replication stuff, which needlessly confuses it, and which still has to go through __init__.

Make _apply faster by not going through __init__.

b77ab46

Add docstring to private _replicate method.

f5f4f91

mhvk force-pushed the skycoord-baseframe-method-speedup branch from 1442114 to f5f4f91 Compare December 22, 2017 16:00

Mention that __init__ is bypassed for slicing and reshaping.

b9633e6

mhvk mentioned this pull request Dec 22, 2017

Speed up __getitem__ and other representation methods. #5598

Closed

eteq approved these changes Dec 23, 2017

View reviewed changes

eteq merged commit 697dfdf into astropy:master Dec 23, 2017

This was referenced Dec 23, 2017

Finalize decision on whether SkyCoord initializer should be bypassed for internal methods #7029

Closed

Re-work / refactor of SkyCoord initializer #7030

Open

mhvk mentioned this pull request Jan 19, 2018

Iterating over a SkyCoord object is very slow #3323

Closed

mhvk deleted the skycoord-baseframe-method-speedup branch June 4, 2019 17:51


		representation_component_units = property(get_representation_component_units)

		def _replicate(self, data, copy=False, **kwargs):

Conversation

mhvk commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taldcroft commented Dec 8, 2017

Uh oh!

astrofrog commented Dec 14, 2017

Uh oh!

mhvk commented Dec 14, 2017

Uh oh!

Cadair commented Dec 14, 2017

Uh oh!

taldcroft commented Dec 14, 2017

Uh oh!

mhvk commented Dec 14, 2017

Uh oh!

Cadair commented Dec 14, 2017

Uh oh!

mhvk commented Dec 14, 2017

Uh oh!

astrofrog commented Dec 14, 2017

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

eteq commented Dec 22, 2017

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

eteq Dec 22, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk commented Dec 22, 2017

Uh oh!

mhvk commented Dec 22, 2017

Uh oh!

adrn commented Dec 22, 2017

Uh oh!

eteq commented Dec 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhvk commented Dec 7, 2017 •

edited

Loading

astropy-bot Bot commented Dec 7, 2017 •

edited

Loading