Speed up creation of simple composite units by mhvk · Pull Request #7649 · astropy/astropy

mhvk · 2018-07-12T23:23:58Z

With this PR, the call that gets done in creating prefix units:

import astropy.units as u
%timeit u.CompositeUnit(1.e-9, [u.m], [1], _error_check=False)
# 100000 loops, best of 3: 6.77 -> 1.05 µs per loop

As this is done a lot during import, it reduces the import astropy.units time by 20% (only the units part, not including the astropy part).

EDIT: I carried on a bit with this. Now:

%timeit u.m / u.s**2
10000 loops, best of 3: 17.3 -> 8.3 µs per loop

Import time reduced from 0.118 to 0.088 s
(Measured by python -X importtime -c "import astropy; import astropy.units" and looking at just the astropy.units part).

astropy-bot · 2018-07-12T23:24:00Z

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

Everything looks good from my point of view! 👍

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

mhvk · 2018-07-12T23:24:36Z

 from .function.logarithmic import *
 from .function import magnitude_zero_points

+from .decorators import *


This change is just so that python -X importtime does not make it appear that importing decorators takes a long time!

It might be worth adding a comment to that effect?

Good point. Added a comment on top.

mhvk · 2018-07-12T23:35:18Z

            if is_effectively_unity(represents.value):
                represents = represents.unit
            else:
-                # cannot use _error_check=False: scale may be effectively unity


This was actually not true: the scale was always checked (and with this PR that check is now very fast).

mhvk · 2018-07-13T21:34:17Z

    if is_effectively_unity(scale):
        return 1.0

-    if np.iscomplex(scale):  # scale is complex


np.iscomplex is awfully slow.

mhvk · 2018-07-13T21:35:12Z

    This ensures that any operation involving a Fraction will use
    rational arithmetic and preserve precision.
    """
-    a_is_fraction = isinstance(a, Fraction)


isinstance(a, Fraction) is awfully slow for the case that a is not a fraction. It might be possible to improve this further...

Can you add a comment to this effect? Something like "checking common cases int and float first because isinstance(a, Fraction) is slow". I can foresee coming back to this code in some time and wanting to "clean it up" :)

Yes, that makes sense. It will also allow us to check easily whether the work-around is in fact needed. I think some of the abc stuff has been rewritten in C now.

mhvk · 2018-08-04T13:38:51Z

@adrn - would you have a chance to review? I ask in part since in #7697 changes were proposed to units/utils.py that are no longer needed if this goes in.

adrn · 2018-08-04T14:39:46Z

    This ensures that any operation involving a Fraction will use
    rational arithmetic and preserve precision.
    """
-    a_is_fraction = isinstance(a, Fraction)


Can you add a comment to this effect? Something like "checking common cases int and float first because isinstance(a, Fraction) is slow". I can foresee coming back to this code in some time and wanting to "clean it up" :)

adrn · 2018-08-04T14:46:15Z

+    if scale.__class__ is float:
+        return scale

+    if scale.imag:


I was surprised that it's safe to do this after only checking is float, but I learned in checking that both int and Fraction() both have the .imag attribute!

Since I also found only by trial & (no) error, I added a comment.

TIL:

>>> (1).imag 0

adrn · 2018-08-04T14:54:41Z

+                                for p in unit.powers]
+            self._scale = sanitize_scale(scale)
+        else:
+            self._scale = scale


It looks like now the scale is only validated in the if clause above - is that ok?

All inputs get adjusted by _expand_and_gather. Since that is far from obvious, I added a comment.

adrn · 2018-08-04T14:55:57Z

                                           bases=represents.unit.bases,
-                                           powers=represents.unit.powers)
+                                           powers=represents.unit.powers,
+                                           _error_check=False)


Sorry for being a bit dense - I just want to make sure I follow the changes to _error_check and the new _error_check=False's added.

In this case, why is it safe to not check?

My logic was that here we know represents is a Quantity, and Quantity instances ensure their unit is properly initialized as a Unit. So, represents.unit is a Unit instance which needs no further checking (note that it has to have scale, bases, and powers attributes). Now possibly this could still go wrong. Since this is not on the fast path for anything anyway, shall I just change it back?

adrn · 2018-08-04T14:56:19Z

                                  bases=s.unit.bases,
-                                  powers=s.unit.powers)
+                                  powers=s.unit.powers,
+                                  _error_check=False)


Here it's safe to not check because the unit is coming from a quantity, so it has already been checked?

Yes, same logic as above. And similarly, not very important for speed.

mhvk · 2018-08-04T19:34:00Z

I pushed a rebased version with more comments; am happy to revert two changes that seem to be perhaps more confusing than it is worth; just let me know.

bsipocz · 2018-10-28T00:12:12Z

@mhvk @adrn - what's the status of this? Can this be merged? If yes, please rebase to make sure the tests are still happy (and label with merge-if-ci-passes). Otherwise please remilestone.

eteq

I had a look and I think this seems good to me. I also checked it locally merged with master since it's been here awhile... No problems. Since @adrn has looked at this and had a few concerns it would be good to get a 👍 from him on this, though...

This gets called all the time for Prefix Units.

mhvk · 2018-10-28T15:37:36Z

I'm happy with this as is.

adrn · 2018-10-30T15:53:07Z

Yep, fine by me!

eteq · 2018-10-30T15:54:47Z

Awesome! I think this is backport-able, do you agree @astrofrog @bsipocz ?

astrofrog · 2018-10-30T16:08:01Z

@eteq - agree!

bsipocz · 2018-10-30T17:22:30Z

I'll do another backport round once #8035 is in.

Speed up creation of simple composite units

This is a regression introduced by astropygh-7649 and reported in astropygh-8260. Before this fix: ``` v2 = 1*u.m**2/u.s**2 (v2 ** (-1/2)).to(u.s/u.m) ``` leads to a unit conversion error because the bases are in the wrong order.

mhvk added units Performance labels Jul 12, 2018

mhvk added this to the v3.1 milestone Jul 12, 2018

mhvk commented Jul 12, 2018

View reviewed changes

mhvk mentioned this pull request Jul 12, 2018

Creation of composite units is relatively slow #7630

Closed

3 tasks

mhvk commented Jul 12, 2018

View reviewed changes

mhvk force-pushed the unit-speed-up-composite-creation branch from 245ae8b to e4308a7 Compare July 13, 2018 04:54

mhvk changed the title ~~Speed up creation of scaled units~~ Speed up creation of simple composite units Jul 13, 2018

mhvk commented Jul 13, 2018

View reviewed changes

pllim requested a review from adrn July 14, 2018 04:38

mhvk mentioned this pull request Aug 4, 2018

Increase coverage #7697

Merged

adrn reviewed Aug 4, 2018

View reviewed changes

mhvk force-pushed the unit-speed-up-composite-creation branch from e4308a7 to 922a854 Compare August 4, 2018 19:33

astrojuanlu mentioned this pull request Aug 7, 2018

Considerations on the performance of astropy.units #7438

Closed

eteq approved these changes Oct 28, 2018

View reviewed changes

mhvk added 6 commits October 27, 2018 22:15

Reorganize units __init__.py a little to help evaluate import times.

ff3ee7e

Special-case and thus speed up creation of scaled units.

6463896

This gets called all the time for Prefix Units.

Avoid composite unit error checking in a few more places.

1e551a7

Further short-cuts for single-unit CompositeUnit initialization.

3a478ca

Unity __hash__ and ensure it is always cached.

07eba2c

Add comments to clarify reasons for ordering.

c39967d

bsipocz force-pushed the unit-speed-up-composite-creation branch from 922a854 to c39967d Compare October 28, 2018 05:16

adrn approved these changes Oct 30, 2018

View reviewed changes

eteq merged commit e7f9cf6 into astropy:master Oct 30, 2018

mhvk deleted the unit-speed-up-composite-creation branch October 31, 2018 00:01

bsipocz pushed a commit that referenced this pull request Oct 31, 2018

Merge pull request #7649 from mhvk/unit-speed-up-composite-creation

538525b

Speed up creation of simple composite units

pllim mentioned this pull request Dec 11, 2018

units: 's / m' and 's / m' are not convertible #8260

Closed

mhvk mentioned this pull request Dec 11, 2018

Fix bug with raising to a negative power. #8263

Merged

Conversation

mhvk commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Aug 4, 2018

Uh oh!

bsipocz commented Oct 28, 2018

Uh oh!

eteq left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Oct 28, 2018

Uh oh!

adrn commented Oct 30, 2018

Uh oh!

eteq commented Oct 30, 2018

Uh oh!

astrofrog commented Oct 30, 2018

Uh oh!

bsipocz commented Oct 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mhvk commented Jul 12, 2018 •

edited

Loading

astropy-bot Bot commented Jul 12, 2018 •

edited

Loading