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

MNT: bump python versions and dependency minimum versions #2169

Merged
merged 5 commits into from
May 9, 2023

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Apr 23, 2023

Rationale

Python 3.8 recently fell off the NEP29 schedule, so this PR removes testing with that and also adds it for python 3.11.
https://numpy.org/neps/nep-0029-deprecation_policy.html

I also bumped versions of other packages used in the minimum versions test as I assume bad things can happen if you bump the python version without doing this! I wasn't sure exactly which versions to use here but mostly went for the oldest that is newer than 2 years. For shapely it seemed safer to go with the older 1.7.1 (August 2020) as 1.8.x had many patch releases stretching into last year. Matplotlib 3.4 is also older than 2 years but 3.5 didn't come out till November 2021.

Implications

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

It looks like there is one MPL_VERSION < (3, 3) check in the tests that could be dropped too. Are we also supposed to update the build-system dependencies in pyproject.toml?

@rcomer
Copy link
Member Author

rcomer commented Apr 25, 2023

Are we also supposed to update the build-system dependencies in pyproject.toml?

That seems likely (I'm by no means an expert on this!) I updated the cython version here to match the min version tests. The setuptools version here is from 2018 and setuptools_scm only last year, so not sure they were previously following the ~2 year policy.

I removed the mpl version check from that test, and also found some handling for older mpl versions in the main code.

# The shading keyword argument was added in MPL 3.3, so keep
# this default updating until we only support MPL>=3.3
default_shading = mpl.rcParams.get('pcolor.shading', 'auto')
default_shading = mpl.rcParams.get('pcolor.shading')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how to read the above comment... Can we get rid of the default altogether now and assume kwargs contains "shading" now?

Suggested change
default_shading = mpl.rcParams.get('pcolor.shading')

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the user can just not pass shading, or am I misunderstanding the question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I was confused thinking there was a default kwarg pcolormesh(*, shading="auto") that we could potentially rely on. But, I think you're right we need to keep this because we are intercepting the user kwargs before going to the super() call.

@@ -44,7 +45,7 @@
from cartopy.mpl.slippy_image_artist import SlippyImageArtist


assert mpl.__version__ >= '3.4', \
assert packaging.version.parse(mpl.__version__).release[:2] >= (3, 4), \
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess mpl v3.10 will ship next year, at which point the string comparison would become problematic.

@greglucas greglucas merged commit 3eeb2c0 into SciTools:main May 9, 2023
@rcomer rcomer deleted the python-versions branch May 30, 2023 20:13
@QuLogic QuLogic added this to the 0.22 milestone Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants