Skip to content

Speed up __getitem__ and other representation methods. - #5598

Closed
mhvk wants to merge 1 commit into
astropy:masterfrom
mhvk:representation-method-speedup
Closed

Speed up __getitem__ and other representation methods.#5598
mhvk wants to merge 1 commit into
astropy:masterfrom
mhvk:representation-method-speedup

Conversation

@mhvk

@mhvk mhvk commented Dec 13, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Inspired by #3323, which notes that iterating over a SkyCoord is very slow, I looked at what happened for representations. For those, 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 input data is known-to-be-good. By skipping this, we can speed up accessing a single element by a factor of 3 to 4.

E.g.,:

import astropy.units as u, numpy as np
from astropy.coordinates import SphericalRepresentation
data = SphericalRepresentation(np.arange(180.)*u.deg, np.arange(-90, 90.)*u.deg, np.arange(180.)*u.kpc)
%timeit for d in data: pass
# 10 loops, best of 3: 70.7 ms per loop

With this PR:

10 loops, best of 3: 20.9 ms per loop

Note: I'm not quite sure this hack for getting a new class item is the right way to go; one might also add a flag to the initializer to skip the checks (or even allow a copy='nocheck' or so).

p.s. This will need some benchmarks...

@mhvk mhvk mentioned this pull request Dec 13, 2016
Closed
@bsipocz

bsipocz commented Dec 13, 2016

Copy link
Copy Markdown
Member

@mhvk - There was once #3337 with similar aims and rather good speed ups. Is there any chance of taking that over and finishing?

@mhvk

mhvk commented Dec 13, 2016

Copy link
Copy Markdown
Contributor Author

@bsipocz - yes, I'm aware of it, especially as it was mostly me that stopped that one. I think part of the problem was that it did too many things at once, so I'll leave this PR to do representations only!

@bsipocz bsipocz added this to the v2.0.0 milestone Mar 23, 2017
@bsipocz

bsipocz commented Mar 23, 2017

Copy link
Copy Markdown
Member

@mhvk - I'm milestoning this into v2.0 to make sure we get these improvements there even if it's not finished up with benchmarks, etc.

@pllim pllim mentioned this pull request May 11, 2017
Closed
@mhvk mhvk mentioned this pull request May 16, 2017
Closed
@mhvk mhvk modified the milestones: v2.0.1, v2.0.0 Jun 12, 2017
@mhvk

mhvk commented Jun 12, 2017

Copy link
Copy Markdown
Contributor Author

While I like these improvements still, I think they should not be done before the proper motion/velocity stuff is implemented, so I'm moving the milestone to 2.0.1 (counting this as a bugfix).

@mhvk mhvk mentioned this pull request Jun 16, 2017
Merged
@bsipocz bsipocz modified the milestones: v2.0.1, v2.0.2 Jul 28, 2017
@astropy-bot

astropy-bot Bot commented Jul 28, 2017 •
edited
Loading

Copy link
Copy Markdown

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

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.

@mhvk

mhvk commented Aug 1, 2017

Copy link
Copy Markdown
Contributor Author

@astrofrog - would it be possible to have astrobot not check PRs that have work-in-progress? In this particular case, I don't really want to add a changelog when I'm not even sure this is the right approach!

@bsipocz

bsipocz commented Aug 2, 2017

Copy link
Copy Markdown
Member

@mhvk - You can always ignore the bot's message, but I'll look into updating the message with something different when the WIP label is set.

@bsipocz bsipocz mentioned this pull request Aug 2, 2017
Merged
@bsipocz bsipocz modified the milestones: v2.0.2, v2.0.3 Sep 7, 2017
@astropy-bot

astropy-bot Bot commented Sep 27, 2017

Copy link
Copy Markdown

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

@bsipocz
bsipocz requested a review from eteq September 27, 2017 12:41
@bsipocz
bsipocz requested a review from adrn September 27, 2017 12:41
@bsipocz

bsipocz commented Sep 27, 2017

Copy link
Copy Markdown
Member

This will need a rebase.

However tests passed before, so should be good to go for a review. @eteq @adrn?

@mhvk
mhvk force-pushed the representation-method-speedup branch from b3d7ef3 to cc6936e Compare September 27, 2017 13:59
@mhvk

mhvk commented Sep 27, 2017

Copy link
Copy Markdown
Contributor Author

OK, I rebased. The main question I have is whether one is better off with the hack I put in now, or by adding a new flag to __init__ that bypasses all checks. Note that the hack may break subclasses that do more than setting attributes, while a new value for the copy flag might throw them off as well (though is should be fine if we ensure it evaluates to False; e.g., we could use copy=None). Or alternatively adjust the meaning of copy=False to not check anything.

@mhvk
Speed up __getitem__ and other representation methods.
8e4c726 
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.
@mhvk
mhvk force-pushed the representation-method-speedup branch from cc6936e to 8e4c726 Compare November 26, 2017 19:50
@bsipocz

bsipocz commented Dec 5, 2017

Copy link
Copy Markdown
Member

@mhvk - Any chance to get this into 2.0.3, or we rather remilestone?

@mhvk mhvk modified the milestones: v2.0.3, v3.0.0 Dec 5, 2017
@mhvk mhvk mentioned this pull request Dec 7, 2017
Merged
@mhvk

mhvk commented Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

closing since part of #6941

@mhvk mhvk closed this Dec 22, 2017
@mhvk
mhvk deleted the representation-method-speedup branch December 20, 2018 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eteq eteq Awaiting requested review from eteq
@adrn adrn Awaiting requested review from adrn

Assignees

No one assigned

Labels

coordinates Performance

Projects

None yet

Milestone

v3.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@mhvk @bsipocz