Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up Unit.to when target unit is the same as original unit #7643

Merged
merged 2 commits into from Jul 12, 2018

Conversation

@astrofrog
Copy link
Member

astrofrog commented Jul 12, 2018

This was split out from #7616

Before

In [1]: from astropy.units import deg

In [3]: %timeit deg.to(deg)
3.69 µs ± 149 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

After

In [3]: from astropy.units import deg

In [4]: %timeit deg.to(deg)
542 ns ± 28.4 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

cc @mhvk

…alue is default (1.0)
@astrofrog astrofrog added this to the v3.1 milestone Jul 12, 2018
@astropy-bot

This comment has been minimized.

Copy link

astropy-bot bot commented Jul 12, 2018

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.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jul 12, 2018

@astrofrog - thanks, this PR is certainly simple and safe. But I wonder if we shouldn't go a little further. My main reason is that using unit.to(other) is really only useful if you want to get the scale factor, and if that is in fact what you expect, then you'll be misled if the unit conversion is not, in fact, a scaling (which would be the case with most equivalencies and with logarithmic units). I think for that case it might be better if unit.to(other) would just raise an exception...

Given this, I wonder if your value=None cannot be used for that purpose, to indicate that one just wants the scale factor if the conversion is, in fact, a simple scaling. If so, the code would be:

if value is None:
    return self._to(other)
else:
    <...current code...>

(Here, self._to(other) short-circuits the case of self is other, so this would serve your purposes.)

A question, then, is whether we can change the default in this way. In favour is my argument above that people are almost certainly using this to get a scale factor and thus will be getting wrong answers if equivalencies are in force. Against is that it changes the API and previously working code might now error (though, as said, I'm not sure that error isn't wanted!).

So, I think this would actually be a good change, but of course in principle we could also do something like:

unity = 1.
def to(self, other, value=unity, equivalencies=[]):
    if value is None:
        return self._to(other)
    if value is unity and other is self:
            return 1.
    <...> current code

But I must say I don't like that much. Instead, if you're unsure about my suggestion, maybe we can for now stick to the PR here, but replace None with unity, so that we ensure the default is the same yet we can make the speed-up. Indeed, another advantage of that is that it is slightly faster since the 1. if value is None else value stanza is no longer needed. (And maybe sphinx renders is more nicely?)

@astrofrog

This comment has been minimized.

Copy link
Member Author

astrofrog commented Jul 12, 2018

@mhvk - I agree that for now the best solution is probably just to set the default to UNITY (which I've done in the last commit)

@mhvk
mhvk approved these changes Jul 12, 2018
Copy link
Contributor

mhvk left a comment

OK, let's go with the low-handing fruit first!

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jul 12, 2018

See #7646 for the question of whether we should change the behaviour.

@mhvk mhvk merged commit 91428ca into astropy:master Jul 12, 2018
7 checks passed
7 checks passed
astropy-bot All checks passed
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on identity-unit at 84.647%
Details
@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jul 12, 2018

Thanks @astrofrog!

@astrofrog astrofrog deleted the astrofrog:identity-unit branch Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.