Skip to content

Speed up SkyCoord initialization by a factor of 2-3 - #7614

Closed
astrofrog wants to merge 2 commits into
astropy:masterfrom
astrofrog:skycoord-perf
Closed

Speed up SkyCoord initialization by a factor of 2-3#7614
astrofrog wants to merge 2 commits into
astropy:masterfrom
astrofrog:skycoord-perf

Conversation

@astrofrog

@astrofrog astrofrog commented Jul 1, 2018

Copy link
Copy Markdown
Member

This implements the caching of representation_info on frames on a class level to speed up initialization of SkyCoords (as described in #2989)

Before

In [2]: %timeit SkyCoord(1, 2, unit='deg', frame='icrs')
1.15 ms ± 54.3 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [5]: ra = np.random.random(1000) * u.deg

In [6]: dec = np.random.random(1000) * u.deg

In [7]: %timeit SkyCoord(ra, dec)
1.17 ms ± 51.5 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

After

In [2]: %timeit SkyCoord(1, 2, unit='deg', frame='icrs')
494 µs ± 63.7 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [6]: ra = np.random.random(1000) * u.deg

In [7]: dec = np.random.random(1000) * u.deg

In [8]: %timeit SkyCoord(ra, dec)
456 µs ± 24.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

There is a speedup even when initializing a single SkyCoord because _get_representation_info gets called 10 times for each SkyCoord initialization, so the caching helps even for a single initialization.

I think in general there are other things in frames that could be classed at a class level, for example representation_component_names and representation_component_units. I don't have time to look into other optimizations, but this one was bugging me since it's such a simple fix.

@astrofrog astrofrog added this to the v3.1 milestone Jul 1, 2018
@astropy-bot

astropy-bot Bot commented Jul 1, 2018 •
edited
Loading

Copy link
Copy Markdown

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

@astrofrog

astrofrog commented Jul 1, 2018

Copy link
Copy Markdown
Member Author

And of course there are complications ;) I'll investigate the test failures.

@astrofrog astrofrog mentioned this pull request Jul 1, 2018
Merged
@astrofrog

astrofrog commented Jul 1, 2018

Copy link
Copy Markdown
Member Author

Ok so there is way too much class magic going on in astropy.coordinates, and this doesn't work. See #7615 for caching on an instance level which still provides a factor of ~2 improvement.

I really think it would be good to tidy up the frame code so that things can be computed once and for all for each frame, and I think there is a fair bit of opportunity there for optimization, but I don't have the time to do this.

I'll leave this open for @eteq and @adrn to see that it doesn't work, but feel free to close.

@astrofrog astrofrog added the Close? Tell stale bot that this issue/PR is stale label Jul 1, 2018
@astrofrog astrofrog mentioned this pull request Jul 1, 2018
Open
@astrofrog

astrofrog commented Aug 19, 2018

Copy link
Copy Markdown
Member Author

I'm going to go ahead and close this

@astrofrog astrofrog closed this Aug 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Close? Tell stale bot that this issue/PR is stale coordinates Performance

Projects

None yet

Milestone

v3.1

Development

Successfully merging this pull request may close these issues.

1 participant

@astrofrog