Skip to content

Speed up SkyCoord initialization by a factor of ~2 - #7615

Merged
eteq merged 1 commit into
astropy:masterfrom
astrofrog:skycoord-perf-2
Jul 12, 2018
Merged

Speed up SkyCoord initialization by a factor of ~2#7615
eteq merged 1 commit into
astropy:masterfrom
astrofrog:skycoord-perf-2

Conversation

@astrofrog

@astrofrog astrofrog commented Jul 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

See #7614 for trying to cache on a class level (which doesn't work but would be ideal)

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 [3]: %timeit SkyCoord(1, 2, unit='deg', frame='icrs')
623 µs ± 26 µ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 [9]: %timeit SkyCoord(ra, dec)
695 µs ± 56.9 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

Also, do I win some kind of prize for being lazy and doing a pull request where all I do is write 'lazy'? ;)

@astrofrog
Cache BaseCoordinateFrame.representation_info on an instance level to…
f70dd72 
… improve SkyCoord initialization performance
@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 😃.

Everything looks good from my point of view! 👍

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

This was referenced Jul 1, 2018
Closed
Open
@astrofrog
astrofrog requested review from adrn and eteq July 1, 2018 13:33
@mhvk

mhvk commented Jul 1, 2018

Copy link
Copy Markdown
Contributor

Ideally, the whole initializer is rewritten, but pending that I think being "lazy" is a good idea!

@astrofrog

astrofrog commented Jul 1, 2018

Copy link
Copy Markdown
Member Author

@mhvk - yeah, this is one of these cases where I'd rather just make such a simple fix in case the refactoring never happens :)

@mhvk

mhvk commented Jul 1, 2018

Copy link
Copy Markdown
Contributor

I'll wait a bit with merging just so @adrn and @eteq can have a look.

@eteq

eteq commented Jul 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

I'm loving the simplicity/laziness! LGTM, so I'm merging.

@eteq
eteq merged commit f6237ca into astropy:master Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eteq eteq eteq approved these changes
@mhvk mhvk mhvk approved these changes
@adrn adrn Awaiting requested review from adrn

Assignees

No one assigned

Labels

coordinates Performance

Projects

None yet

Milestone

v3.1

Development

Successfully merging this pull request may close these issues.

3 participants

@astrofrog @mhvk @eteq