-
Notifications
You must be signed in to change notification settings - Fork 391
MNT: bump minimum dependency versions #2623
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
Conversation
| # See LICENSE in the root of the repository for full licensing details. | ||
|
|
||
| import matplotlib | ||
| import packaging.version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place we used packaging in the main library and the reason it is listed as a dependency. So we could move packaging back to being only a test dependency.
greglucas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor suggestions, but it looks like a really nice cleanup!
| matrix: | ||
| os: [ubuntu-latest, macos-latest, windows-latest] | ||
| python-version: ['3.10', '3.11', '3.12', '3.13'] | ||
| python-version: ['3.11', '3.12', '3.13'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we added 3.14 in #2556 but that looks like we only added wheels but didn't add any testing. I guess we can do that as a follow-up PR to add that to our rotation unless you want to do it here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to add it but got nervous because I assumed there was a reason it didn't go in with #2556! I'll add it and see what breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split that out to #2624, as it is a little more than just adding to the matrix.
.github/workflows/ci-testing.yml
Outdated
| include: | ||
| - os: ubuntu-latest | ||
| python-version: '3.11' | ||
| python-version: '3.12' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is in the middle? I feel like we should push this to the latest if we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't the latest when we added it at #2371 but I don't remember if there was a reason. Happy to update to latest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, possibly it wasn't the latest so it didn't run the coverage step?
environment.yml
Outdated
| dependencies: | ||
| - cython>=0.29.28 | ||
| - numpy>=1.23 | ||
| - cython>=0.30.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The min-ver CI test has Cython 3.0 as the version number, was this a typo or did you want 0.30?
| - cython>=0.30.0 | |
| - cython>=3.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops! Yes, typo. v3 came out in July 2023. I guess they changed their numbering convention at that point as there was never a v1 or v2.
https://pypi.org/project/Cython/#history
lib/cartopy/mpl/geoaxes.py
Outdated
| # Currently pcolor_col.get_array() will return a compressed array | ||
| # and warn unless we explicitly set the 2D array. This should be | ||
| # unnecessary with future matplotlib versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you look up if this was updated in Matplotlib and can be removed or what version this can be removed in so we can put a note here for future searching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am struggling to find the relevant commit/PR, but the warning is still there at v3.9.4 and at v3.10.0 PolyQuadMesh no longer has its own get_array. I'll add a note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates in this file are very satisfying :) MPL 3.8 had a lot of nice quadmesh updates that we are finally able to take full advantage of!
lib/cartopy/mpl/path.py
Outdated
| # make_compound_path handling for empty paths was added at | ||
| # https://github.com/matplotlib/matplotlib/pull/25252 | ||
| paths.append(path) | ||
| paths.append(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be written as a list comprehension now.
return Path.make_compound_path([shapely_to_path(geom) for geom in shape.geoms])
pyproject.toml
Outdated
| "wheel", | ||
| "setuptools >= 77.0.3", | ||
| "Cython >= 0.29.24", | ||
| "Cython >= 0.30", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Cython >= 0.30", | |
| "Cython >= 3.0", |
|
Sorry I hit the approve button on the wrong PR in my inbox! I'm inclined to agree though - it looks good! |
4288b79 to
47eb95d
Compare
|
OK fiona is still not available for python v3.14 Toblerity/Fiona#1504 |
Rationale
Bump minimum supported python version to 3.11, and other dependency versions to something newer but always at least 2 years old. I only looked at packages that we pin in the minimum version tests, although we do specify minimums for other packages.
Implications
Dropping support for mpl 3.7 allows us to delete a bunch of old code 😎