Skip to content

Speed up bottleneck in some differential unit lookup#7924

Merged
eteq merged 2 commits intoastropy:masterfrom
adrn:coordinates/deriv-key-speedup
Oct 19, 2018
Merged

Speed up bottleneck in some differential unit lookup#7924
eteq merged 2 commits intoastropy:masterfrom
adrn:coordinates/deriv-key-speedup

Conversation

@adrn
Copy link
Member

@adrn adrn commented Oct 19, 2018 •
edited
Loading

Representation objects store differential (velocity) data in a dictionary .differentials with keys set to the SI unit with which the differential is take. So, for velocities, this is 's', because velocities are time derivatives of positional information.

This PR makes a small change to the way BaseDifferential._get_deriv_key() gets the differentials dictionary key from specified differential and representation objects. For scalar input, this makes _get_deriv_key() ~20x faster, which makes representation.with_differentials() ~16x faster, which makes frame initialization ~7x faster (which also propagates to some paths through the SkyCoord initializer) when there is velocity data.

Defining:

rep = coord.CartesianRepresentation([1., 2, 3] * u.kpc)
dif = coord.CartesianDifferential([1, 2, 3.] * u.km/u.s)

This PR:

%timeit dif._get_deriv_key(rep)
238 µs ± 18.2 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
%timeit rep.with_differentials(dif)
662 µs ± 58.6 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
%timeit coord.ICRS(ra=1*u.deg, dec=2*u.deg, pm_ra_cosdec=1*u.mas/u.yr, pm_dec=2*u.mas/u.yr)
1.23 ms ± 140 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

Master:

%timeit dif._get_deriv_key(rep)
5.41 ms ± 123 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
%timeit rep.with_differentials(dif)
10.8 ms ± 329 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
%timeit coord.ICRS(ra=1*u.deg, dec=2*u.deg, pm_ra_cosdec=1*u.mas/u.yr, pm_dec=2*u.mas/u.yr)
7.22 ms ± 969 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

@adrn adrn added this to the v3.1 milestone Oct 19, 2018
@adrn adrn requested review from eteq and mhvk October 19, 2018 03:21
@astropy-bot
Copy link

astropy-bot bot commented Oct 19, 2018 •
edited
Loading

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

@pllim
Copy link
Member

pllim commented Oct 19, 2018

Add a benchmark to https://github.com/astropy/astropy-benchmarks/blob/master/benchmarks/coordinates.py ?

eteq
eteq approved these changes Oct 19, 2018
View reviewed changes
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

LGTM except that it needs a changelog entry that highlights the performance boost. (I'm fine with doing that now so we can merge this soon but then also following @pllim's suggestion of a benchmark, but possibly later)

@adrn adrn mentioned this pull request Oct 19, 2018
Merged
@adrn
Copy link
Member Author

adrn commented Oct 19, 2018

See astropy/astropy-benchmarks#68 for benchmarks!

@eteq
Copy link
Member

eteq commented Oct 19, 2018

LGTM, so merging so as not to block some other planned coordinates enhancements.

As a side-note, we may want to re-work some of these preformance enhancements notes into a dedicated section, hence the "what's new needed" label. But not the end of the world if we don't get to that by release time.

@eteq eteq merged commit 66c7967 into astropy:master Oct 19, 2018
@mhvk
Copy link
Contributor

mhvk commented Oct 20, 2018

Belatedly: nice find! And does all look good.

@adrn adrn mentioned this pull request Oct 24, 2018
Merged
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 Awaiting requested review from mhvk

Assignees

No one assigned

Labels

coordinates Performance whatsnew-needed

Projects

None yet

Milestone

v3.1

Development

Successfully merging this pull request may close these issues.

4 participants

@adrn @pllim @eteq @mhvk