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

Modularize Quantity helpers, ensuring scipy.special and erfa are loaded on demand #7636

Merged
merged 10 commits into from Sep 21, 2018

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Jul 10, 2018

This PR has two purposes. The main one is to ensure that scipy.special is only loaded on demand, so that for a typical situation in which astropy is used but scipy.special is not, the latter does not get imported unless one of its functions are actually applied to a Quantity. This helps reduce the import time of astropy.units quite a bit (see below). The second purpose is to split up the very large quantity_helpers file into more manageable pieces, by making it a module.

Note that as part of this, it is becoming possible to "register" sets of ufuncs with the units module without actual loading - I hope to use this this is already used here for erfa, but it could be exposed as well. I've not written any documentation for that yet, as I first want to be sure people agree the structure makes sense (also, it sets the format for the helper functions in stone, which I'm not sure I like).

Excerpts from python -X importtime -c 'import astropy.units:

# With current master
import time: self [us] | cumulative | imported package
import time:       787 |      60205 |       scipy.special
import time:       419 |      69729 |     astropy.units.quantity_helper
import time:      4641 |      76212 |   astropy.units.quantity
import time:     20671 |     521538 | astropy.units
# With this PR
import time:       216 |        216 |       astropy.units.quantity_helper.converters
import time:       310 |        310 |       astropy.units.quantity_helper.helpers
import time:       183 |        183 |       astropy.units.quantity_helper.scipy_special
import time:       225 |        932 |     astropy.units.quantity_helper
import time:      5404 |       8088 |   astropy.units.quantity
import time:     19827 |     432185 | astropy.units

@astropy-bot
Copy link

astropy-bot bot commented Jul 10, 2018

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.



UFUNC_HELPERS = UfuncHelpers()
UNSUPPORTED_UFUNCS = UFUNC_HELPERS.UNSUPPORTED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This becomes an unnecessary link, but keeping it for backwards compatibility (currently, it is also still being used in helpers for unsupported numpy ufuncs; can update that to use setting to None instead if people like this whole PR).

@mhvk mhvk requested review from astrofrog, adrn and bsipocz July 10, 2018 18:07
@mhvk
Copy link
Contributor Author

mhvk commented Jul 11, 2018

circle-ci failure is a false positive - something to do with downloading again, so cc @pllim and @bsipocz.

@pllim
Copy link
Member

pllim commented Jul 12, 2018

Re: CircleCI -- The error suggests something funky is going on with the cache lock in that CircleCI run. If restarting it makes it go away, I am not sure how to debug it... 🤷‍♀️

@pllim
Copy link
Member

pllim commented Jul 14, 2018

This needs a rebase anyway. Let's see if CircleCI fixes itself...

@mhvk mhvk force-pushed the quantity-modularize-helpers branch from 13dcee0 to d4971e0 Compare July 14, 2018 05:14
@mhvk
Copy link
Contributor Author

mhvk commented Jul 14, 2018

OK, rebased... Hopefully things will pass now.

@pllim
Copy link
Member

pllim commented Jul 14, 2018

Travis is still running but CircleCI already passed. Not sure what caused the previous failure!

@bsipocz bsipocz removed their request for review July 14, 2018 13:32
@bsipocz
Copy link
Member

bsipocz commented Jul 14, 2018

@mhvk - I've removed myself from the reviewers, as I'm not familiar enough with units internal to spot anything that you might have overseen.

However, I very much like the general idea of splitting up the helpers and make sure we don't import big things unnecessarily.

@mhvk mhvk force-pushed the quantity-modularize-helpers branch from d4971e0 to b900cfc Compare August 13, 2018 19:07
@mhvk mhvk changed the title Modularize Quantity helpers, ensuring scipy.special is loaded on demand Modularize Quantity helpers, ensuring scipy.special and erfa are loaded on demand Aug 14, 2018
@mhvk
Copy link
Contributor Author

mhvk commented Aug 14, 2018

Rebased since erfa had become supported - that is now modularized as well, for those who use units but not coordinates.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

I'm not an expert on astropy.units but these changes seem worthwhile to me. It's hard to see changes vs code that was moved, so I can't review everything line by line, but I think this is fine.

@mhvk mhvk force-pushed the quantity-modularize-helpers branch from 6c11ba4 to 830baee Compare September 18, 2018 17:18
@mhvk mhvk merged commit b3cf73f into astropy:master Sep 21, 2018
@mhvk mhvk deleted the quantity-modularize-helpers branch September 21, 2018 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants