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

TST: Skip scipy tests if scipy isn't installed #2282

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

greglucas
Copy link
Contributor

Scipy isn't a required dependency, so skip the tests that require scipy if scipy isn't found.

pytest.importorskip in the module-level imports caused flake8 warnings, so I decided to just try/except and skip instead which passes flake8 without requiring a bunch of extra ignores.

closes #2275

from scipy.interpolate import NearestNDInterpolator
from scipy.signal import convolve2d
except ImportError:
pytest.skip("scipy is required for contouring", allow_module_level=True)
Copy link
Member

@rcomer rcomer Oct 29, 2023

Choose a reason for hiding this comment

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

I think contour itself does not need scipy, but specifically test_contour_linear_ring uses those two imported objects. So I suggest moving the imports within that test and putting the skip just there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Moved that down into the test.

try:
import scipy.spatial
except ImportError:
pytest.skip("scipy is required for image transforms", allow_module_level=True)
Copy link
Member

Choose a reason for hiding this comment

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

In here, everything except test_regridding_with_invalid_extent will pass providing you have at least one of scipy or pykdtree installed. Is it worth modifying this to try both imports? Or is it too unlikely that a developer would have pykdtree but not scipy?

I am also unclear how to add appropriate skips to the parametrization of test_regridding_with_invalid_extent if we did that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it is simple enough to do the nested try/except and put the skip only at the nested version. I also added a skip to the parameterization if scipy can't be imported there.

rcomer
rcomer previously approved these changes Oct 30, 2023
Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

If I run without scipy or pykdtree, I get failures in test_examples.py and test_imges.py. That could arguably be out of scope of this PR though.

@greglucas
Copy link
Contributor Author

If I run without scipy or pykdtree, I get failures in test_examples.py and test_imges.py. That could arguably be out of scope of this PR though.

Good catch, I didn't realize I had pykdtree hanging around still. In fact, I get even more ERRORS due to image dependency imports in other modules like the ows tests. This is adding a lot of conditional logic for the skips in many places at this point considering no one else has complained about this before! I'm not sure how valuable this is...

@greglucas greglucas marked this pull request as draft November 1, 2023 02:10
@dopplershift
Copy link
Contributor

I think it's definitely helpful given that neither of these are considered required, so IMO we should be able to run the test suite successfully without them.

@dopplershift
Copy link
Contributor

I guess the alternative is to add one/both to the "test" optional dependencies.

@greglucas greglucas marked this pull request as ready for review November 2, 2023 13:20
@greglucas
Copy link
Contributor Author

Alright, I refactored it a bit and tested this with the various combinations of pykdtree/scipy available/not available so that there are no errors.

There are a few problems that were slightly annoying and made this maybe a little more verbose than ideal:

  1. We import our img_transform in other modules, for example to get _OWS_LIB_AVAILABLE we go through that entire module that has an import of img_transform, which requires us to skip above that import statement. This also means we can't set pytestmark = requires_pykdtree_scipy at the top of the module because that occurs after the module imports.
  2. We have optional dependencies on two imports scipy or pykdtree, so pytest.importorskip I don't think will work in that situation. So I added a _HAS_PYKDTREE_SCIPY variable that we then use to just apply a pytest.skip() early on in the test module imports.

@greglucas greglucas dismissed rcomer’s stale review November 2, 2023 13:26

Significant changes and scope expansion since this approval

@QuLogic
Copy link
Member

QuLogic commented Dec 3, 2023

Seems to still be conflicts now?

lib/cartopy/tests/test_img_transform.py Outdated Show resolved Hide resolved
Comment on lines +9 to 19
import pytest


try:
import scipy # noqa: F401
except ImportError:
pytest.skip("scipy is required for vector transforms", allow_module_level=True)


import cartopy.crs as ccrs
import cartopy.vector_transform as vec_trans
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import pytest
try:
import scipy # noqa: F401
except ImportError:
pytest.skip("scipy is required for vector transforms", allow_module_level=True)
import cartopy.crs as ccrs
import cartopy.vector_transform as vec_trans
from cartopy.tests.conftest import requires_scipy
import cartopy.crs as ccrs
import cartopy.vector_transform as vec_trans
pytestmark = requires_scipy

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 one unfortunately doesn't work because import cartopy.vector_transform raises an import error before the pytestmark takes effect.

rcomer
rcomer previously approved these changes Dec 15, 2023
lib/cartopy/tests/mpl/test_features.py Outdated Show resolved Hide resolved
@rcomer rcomer dismissed their stale review December 18, 2023 08:33

Forgot I had lingering outdated comment

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

Sorry for previously messed up review - I started adding comments a couple of weeks ago but then PRed that change instead. Then forgot I still had the comment here when I approved it. Can’t seem to delete the comment now either.

Anyway, I think this is good to go.

@QuLogic
Copy link
Member

QuLogic commented Jan 3, 2024

Needs a rebase.

If a user doesn't have scipy or pykdtree installed on their system
they can not import image transform modules, so we also need to
skip these tests for them.
@greglucas greglucas merged commit da6a8c5 into SciTools:main Jan 3, 2024
22 checks passed
@greglucas greglucas deleted the tst-skip-scipy branch January 3, 2024 15:10
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.

Tests fail without scipy
4 participants