A few speedups related to SkyCoord.__getitem__ by embray · Pull Request #3337 · astropy/astropy

embray · 2015-01-22T19:36:12Z

Here's a small collection of speedups aimed at the test case from #3323. These are very specifically aimed at that one case, but may help in other cases as well:

In [1]: import numpy as np
In [2]: from astropy import units as u
In [3]: from astropy.coordinates import SkyCoord
In [4]: ra = np.random.uniform(0, 360, size=1000) * u.deg
In [5]: dec = np.random.uniform(-90, 90, size=1000) * u.deg
In [6]: crd = SkyCoord(ra, dec)
In [7]: %timeit [c for c in crd]
1 loops, best of 3: 19.3 s per loop

with the speedups in this branch I now get:

In [3]: %timeit [c for c in crd]
1 loops, best of 3: 2.47 s per loop

so improvement by an order of magnitude. There are plenty other things that could be done, but I shouldn't spend any more time on it right now (I may come back to it later). There are also a few open questions I left in the code as # TODO comments that might be worth a look.

One of them in particular is that this obviates the need for the FRAME_ATTR_NAMES_SET function, which I think could either be removed outright (since it isn't documented), or at least deprecated.

…ke frame_set. Use this to implement the FRAME_ATTR_NAMES_SET() 'macro' which speeds it up significantly (it could be removed entirely at this point, or at least deprecated, though it's used so frequently in the sky_coordinate module that it might still be useful to have a module-level alias to it)

… each attribute as a lambda. Might be nice to speed this up on the class-level as well which might be doable in some cases since frame attributes, for example, are per-class. Still trickier though.

…be calculated repeatedly in the loop. However, also removed a variable (n_attr_names) that was something of an unnecessary and premature 'optimization'

…ngle, and a few workarounds to performance shortcomunings in units.

embray · 2015-01-22T19:38:38Z

The performance gains from this change are marginal in most cases, but in a loop it does make a difference when the same attributes are looked up over and over again.

embray · 2015-01-26T16:48:26Z

That's weird that those table tests failed. Not sure what that's about (none of the actual direct tests of the coordinates package failed). I'll have a look at it later.

mhvk · 2015-01-26T17:31:17Z

@embray - I don't quite understand why the speed-ups to Angle and Longitude should be necessary in your example. Your example should only take elements from the Longitude array, so why is the wrapping relevant at all? Why does the wrap angle even get set? It seems somewhere the code reinitializes Longitude and Latitude when there is no need to. Do you know where this is? (Sorry, while I know Angle and its subclasses well, I'm not very familiar with Coordinates.)

embray · 2015-01-26T18:00:23Z

@mhvk Currently (and this in of itself is probably not ideal--I was just adding what speedups I could without otherwise changing the current logic), every time SkyCoord(self) is called, the initializer calls _parse_coordinate_arg which in turn calls coords.data.represent_as(frame.representation), which in turn ends up instantiating a new SphericalRepresentation every time, which initializes a new Latitude and Longitude here (though by default I think it's passing in existing Latitude and Longitude objects, which could probably be initialized faster when copy=True) but I'm not sure.

embray · 2015-01-26T18:01:53Z

Of all the changes in this code this is one of the most relevant. But though instantiating the default wrap angle over and over again also hurt quite a bit in a loop.

Fine to keep this of course! (Am slightly dismayed that it matters so much...)

Not a huge amount but it was still an improvement.

Interestingly, instantiating an Angle from an existing Angle is no faster than instantiating one from a Quantity (such as 360 * u.deg). In fact it may even be a smidge slower. That's not including the instantiation of the Quantity itself. We're talking Angle(a) vs. Angle(q). (not to say it's terribly slow in the first place)

I'm looking at Angle initialisation right now. Hope to have a PR to your branch shortly...

mhvk · 2015-01-27T18:28:25Z

@embray - after comments in #3323, I realised that the angle stuff will all be unnecessary if in BaseRepresentation.__getitem__ one would not use the class initialiser but instead have a class method that simply sets the required attributes..

embray · 2015-01-27T18:41:01Z

At least some of the speedups in Angle still seem worthwhile regardless.

mhvk · 2015-01-27T18:59:16Z

@embray - agreed that some are good to do anyway. I mostly don't like the ones that have TODO comments.

mhvk · 2015-01-27T18:59:37Z

These changes are all fine.

mhvk · 2015-01-27T19:05:15Z

p.s. My guess would be that much of the gain of the TODO items could be made by optimizing Angle.__new__ (just like Quantity.__new__ could be sped up, but I never got around to doing Angle as well).

embray · 2015-01-27T20:35:54Z

I agree on that last comment--I thought of looking into it but Angle.__new__ went into Quantity.__new__ and it was just a rabbit hole so I stopped looking, at least at the time.

mhvk · 2015-01-27T21:22:53Z

We tried very hard to make sure Quantity.__new__ is as fast as possible if the input value is a Quantity, type(value) is cls, no unit or dtype change is necessary, and copy=False. I think for any Angle(angle), Quantity.__new__ should be about as fast as one can hope for, with the main speed-up possible by adding copy=False (if this is reasonable, e.g., if one knows a copy has already been made or is otherwise not needed).

embray · 2015-01-30T19:59:03Z

I'm still having a hard time understanding the test failures here, particularly in the table_mixin tests. It seems that test_join passes for regular tables, but then fails when using MaskedTable or any other Table subclass. It's strange, really...

mhvk · 2015-01-30T20:30:43Z

Yes, that fails also with the changes I made... Unfortunately, the table tests are so convoluted with things being set up in conftest.py that it is very hard to reproduce on a simpler example. It does seem that there somehow are too many sky coordinates.

embray · 2015-01-30T21:30:00Z

Well, it turns out all the table failures are related to my "enhancements" to SkyCoord.__getattr__. What happens seems to have something to do with the self._sky_coord_frame switcheroo that happens in SkyCoord.__getitem__. In the case of some of the table tests, the original self._sky_coord_frame is being sliced, so this replaces it with a new frame. However, any cached attribute lookups on self are now referencing the wrong frame, which leads to problems.

The attribute lookup speedups are not insignificant, but they're not the most significant improvement in this PR either. So if it ends up overcomplicating things I'll just remove it. Though the way SkyCoord.__getitem__ works currently still seems ripe for improvement...

eteq · 2015-02-04T23:02:20Z

@embray - other than the attribute lookup stuff this should be good, right? So maybe you should just remove that part so we can get partial speedups in for 1.0, and then try to get the attribute speedups as a later PR for 1.0.x?

Or might this be a bit too late for 1.0 anyway, given that it's doing quite a bit deep in the plumbing of SkyCoord...?

astrofrog · 2015-02-04T23:11:56Z

Given that this is still failing on the CI I'd actually just vote to include it in 1.0.1 (I think we ruled in the past that performance fixes were ok in minor releases because they don't change the API).

eteq · 2015-02-04T23:16:25Z

ok, sounds good - will assign it that milestone then.

landscape-bot · 2015-02-05T20:58:12Z

Repository health increased by 10% when pulling 4399914 on embray:coordinates/minor-speedups into 0233c22 on astropy:master.

266 new problems were found (including 5 errors and 257 code smells).
1543 problems were fixed (including 1244 errors and 182 code smells).

embray · 2015-02-06T19:27:50Z

Yeah, I think it's best to delay this for now. I have thoughts on how to fix the attribute lookup thing so that it can still be benefitted from.

Meanwhile, what the heck is landscape-bot?

mhvk · 2015-03-20T14:31:20Z

@embray - I don't particularly like this change -- it adds complexity when at least for larger arrays the main culprit in making things slow is that the conversion has to involve making a copy (the scale determination is very fast, especially if units are the same). Also, my feeling is that this is the wrong place to optimize this -- the converter shouldn't even be called when the units are the same.

I agree, I just haven't had a chance to work on this more.

embray · 2015-04-08T16:25:42Z

Dropping this from the milestone until I have a chance to address the issues here.

bsipocz · 2017-05-09T19:53:04Z

@eteq @adrn @mhvk - This branch still seems to be faster than current master. Is there any chance to cherry-pick stuff from here to get things moving and close this PR?

mhvk · 2017-05-09T19:59:21Z

Definitely a good idea, though I sadly don't see myself having time to work on this soon.

pllim · 2017-05-11T18:44:35Z

Conflicts and seems to have been replaced by #5598

bsipocz · 2017-05-11T18:53:34Z

@pllim - is that PR have everything from here?

pllim · 2017-05-11T18:57:50Z

Actually no, but this has a better chance of being completed if @mhvk "absorbs" it rather than sitting around here.

mhvk · 2017-05-11T20:30:34Z

If this is closed, I'll simply never see it anymore, so I'll leave it open for now. I'll also set the milestone to 3.0; if this is seen then and nothing happened, just close.

astropy-bot · 2017-09-27T12:16:35Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 2 years. 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.

mhvk · 2017-09-27T13:59:17Z

At this stage it is probably best to raise issues for the various fixes that were proposed here, and that were not yet implemented.

astropy-bot · 2017-11-21T11:13:51Z

⏰ 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.

embray added 4 commits January 22, 2015 12:52

Speed up SkyCoord.__getattr__ calls by caching the method for getting…

5a65ae6

… each attribute as a lambda. Might be nice to speed this up on the class-level as well which might be doable in some cases since frame attributes, for example, are per-class. Still trickier though.

One loop is sped up by saving a boolean variable that didn't need to …

2217917

…be calculated repeatedly in the loop. However, also removed a variable (n_attr_names) that was something of an unnecessary and premature 'optimization'

Speed up Longitude initialization a bit by including a default wrap_a…

3d361f3

…ngle, and a few workarounds to performance shortcomunings in units.

embray added coordinates units Enhancement Performance and removed Enhancement labels Jan 22, 2015

embray reviewed Jan 22, 2015
View reviewed changes

embray reviewed Jan 26, 2015
View reviewed changes

cdeil mentioned this pull request Jan 27, 2015

Iterating over a SkyCoord object is very slow #3323

Closed

mhvk reviewed Jan 27, 2015
View reviewed changes

Comment thread astropy/coordinates/angles.py

mhvk Jan 27, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are all fine.

Speeds up instantiating an Angle from an existing Angle object.

322a57d

Faster Angle initialisation.

f743613

eteq added this to the v1.0.1 milestone Feb 4, 2015

embray modified the milestones: v1.0.2, v1.0.1 Mar 4, 2015

mhvk reviewed Mar 20, 2015
View reviewed changes

astrofrog assigned eteq Mar 21, 2015

embray removed this from the v1.0.2 milestone Apr 8, 2015

embray force-pushed the master branch from 63540e0 to 34f4c29 Compare October 5, 2015 21:18

mhvk mentioned this pull request May 22, 2016

Faster Angle initialisation. #4970

Merged

bsipocz mentioned this pull request Dec 13, 2016

Speed up __getitem__ and other representation methods. #5598

Closed

pllim closed this May 11, 2017

mhvk reopened this May 11, 2017

mhvk added this to the v3.0.0 milestone May 11, 2017

astropy-bot Bot closed this Nov 21, 2017

astrofrog added the closed-by-bot Closed by stale bot label Nov 21, 2017

Conversation

embray commented Jan 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

embray commented Jan 26, 2015

Uh oh!

mhvk commented Jan 26, 2015

Uh oh!

embray commented Jan 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jan 27, 2015

Uh oh!

embray commented Jan 27, 2015

Uh oh!

mhvk commented Jan 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jan 27, 2015

Uh oh!

embray commented Jan 27, 2015

Uh oh!

mhvk commented Jan 27, 2015

Uh oh!

embray commented Jan 30, 2015

Uh oh!

mhvk commented Jan 30, 2015

Uh oh!

embray commented Jan 30, 2015

Uh oh!

eteq commented Feb 4, 2015

Uh oh!

astrofrog commented Feb 4, 2015

Uh oh!

eteq commented Feb 4, 2015

Uh oh!

landscape-bot commented Feb 5, 2015

Uh oh!

embray commented Feb 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

embray commented Apr 8, 2015

Uh oh!

bsipocz commented May 9, 2017

Uh oh!

mhvk commented May 9, 2017

Uh oh!

pllim commented May 11, 2017

Uh oh!

bsipocz commented May 11, 2017

Uh oh!

pllim commented May 11, 2017

Uh oh!

mhvk commented May 11, 2017

Uh oh!

astropy-bot Bot commented Sep 27, 2017

Uh oh!

mhvk commented Sep 27, 2017

Uh oh!

astropy-bot Bot commented Nov 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone