Avoid importing astropy.stats for astropy.modeling#7936
Conversation
|
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
left a comment
There was a problem hiding this comment.
This makes sense, but just to ensure things stay consistent, maybe add a test? Or am I overly paranoid??
|
@mhvk - you mean to check the value of GAUSSIAN_SIGMA_TO_FWHM is the same in both modules? |
|
Yes, that's what I meant. Really shouldn't change, of course... |
|
@mhvk - added! |
| # Note that we define this here rather than using the value defined in | ||
| # astropy.stats to avoid importing astropy.stats every time astropy.modeling | ||
| # is loaded. | ||
| GAUSSIAN_SIGMA_TO_FWHM = 2.0 * np.sqrt(2.0 * np.log(2.0)) |
There was a problem hiding this comment.
Doesn't quite sit well with my OCD to have the same thing defined in multiple places. Is it possible to move this to constants or does that also introduce import time penalty? Anyways, just a thought.
There was a problem hiding this comment.
Practicality beats purity
This removes the only import of astropy.stats in astropy.modeling, which speeds up the import of astropy.modeling from ~1.8 seconds to ~1.5 seconds on my machine (as profiled with
-X importtime). I noticed this while debugging an issue in astropy-benchmarks which is caused by importing astropy.modeling ultimately importing pytest. This doesn't fix the root cause (see #7932 for that) but avoids a long chain of imports that isn't needed.