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

Prepare for Numpy 2.0 #2338

Merged
merged 2 commits into from
Mar 10, 2024
Merged

Prepare for Numpy 2.0 #2338

merged 2 commits into from
Mar 10, 2024

Conversation

greglucas
Copy link
Contributor

I installed the latest pre-release of numpy 2.0 to see if we had any failures appear and these were the only ones that showed up. Pretty simple fixes to all internal/test machinery.

Here is the quick command to help others wanting to test this out by upgrading some local packages as well if wanted.
python -m pip install numpy matplotlib contourpy --index-url https://pypi.anaconda.org/scientific-python-nightly-wheels/simple --pre --upgrade numpy

@greglucas greglucas added this to the Next Release milestone Mar 5, 2024
Comment on lines 58 to 60
if longitude != 180:
# Wrap the longitude to the range -180 to 180, keeping positive 180s
longitude = ((longitude + 180) % 360) - 180
Copy link
Member

Choose a reason for hiding this comment

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

There's a minor difference here in that 180 + 360 *n was formerly considered +180, whereas now it'll be -180.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I was ignoring the larger multiples. I think we need an intermediate variable to handle that as before, I just pushed up a new change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking more generally here... this is only in the W/E labels function. Do we actually want a label on the 180s, or should we just strip that off altogether like we do for 0 already? Remove the E/W ambiguity.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, that might be something to do instead. I feel like there are probably differing opinions on which one to use, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, there was one image failure with that update, so I pushed the change to remove the E/W label as that simplifies the logic a bit. I'm happy to remove that and go back to the previous logic if preferred too.

I looked up a few examples and it seems there are versions with and without the labels on 180, so no standard I could see at a quick glance.

Matlab does produce E/W labels on 180: https://www.mathworks.com/help/map/the-map-grid.html
Images without the E/W labels on 180: https://manoa.hawaii.edu/exploringourfluidearth/physical/world-ocean/locating-points-globe

Wikipedia seems to indicate you should label both the prime and antimeridian with either E or W: https://en.wikipedia.org/wiki/180th_meridian

The longitude at this line can be given as either east or west.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was too undecided. I now went to the old version and kept the E/W labels as they were so we aren't changing anything anymore. This is the simple change for now to get ready for numpy 2.0 and we can re-evaluate the label naming in a follow-up PR later.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'd eventually want to go for something "smarter", that adds the direction if there are (potential for) two 180 labels. But this can be a separate PR.

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@QuLogic QuLogic merged commit bc7b373 into SciTools:main Mar 10, 2024
23 checks passed
@greglucas greglucas deleted the numpy-2.0 branch March 10, 2024 00:08
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