move scipy imports#12192
Conversation
a6b2767 to
8a7fff5
Compare
dhomeier
left a comment
There was a problem hiding this comment.
LGTM; could probably need some explanation on the motivation of these changes in the description, but AFAICT does what it is supposed to do.
| ############################################################################## | ||
|
|
||
|
|
||
| @pytest.mark.skipif(HAS_SCIPY, reason="can't have :mod:`scipy` imported") |
There was a problem hiding this comment.
No need to be too fancy with the reason here. It doesn't even show up in the test log and definitely will not be rendered by anything user facing. It is just a note for devs.
| @pytest.mark.skipif(HAS_SCIPY, reason="can't have :mod:`scipy` imported") | |
| @pytest.mark.skipif(HAS_SCIPY, reason="scipy is installed") |
There was a problem hiding this comment.
Yes, just at very first reading it did not make much sense to me.
There was a problem hiding this comment.
On a related note, this test doesn't seem to be running! I don't see any reason why it doesn't run (except when scipy is installed)!
There was a problem hiding this comment.
You mean, it doesn't run in CI or locally? Check the pytest header. Maybe somehow all our jobs pull in scipy.
There was a problem hiding this comment.
Ohh! We should fix that. Codecov can merge the coverage reports.
Followup PR?
There was a problem hiding this comment.
Add coverage to Python 3.9 with minimal dependencies?
There was a problem hiding this comment.
change it to
- name: Python 3.9 with minimal dependencies and full coverage
os: ubuntu-latest
python: 3.9
toxenv: py39-test-cov-clocale
There was a problem hiding this comment.
Yes, let's explore that as a separate PR. 😸
There was a problem hiding this comment.
Yes, testing both minimal and full dependencies sounds sensible.
Wondering how is this going to be evaluated, e.g. if one job reports increased and the other decreased coverage... 👻
Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
8a7fff5 to
269d99f
Compare
Don't import every time the function is called.
Edit: this is mostly a stylistic change (as the import is fairly inexpensive), but this implementation is better because it separates the logic of optional dependencies from the actual physics.
Signed-off-by: Nathaniel Starkman (@nstarman) nstarkman@protonmail.com
Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
Extra CIlabel.no-changelog-entry-neededlabel. If this is a manual backport, use theskip-changelog-checkslabel unless special changelog handling is necessary.astropy-botcheck might be missing; do not let the green checkmark fool you.backport-X.Y.xlabel(s) before merge.