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

Avoid a deprecation warning from numpy 1.25 in crs._safe_pj_transform #2194

Merged

Conversation

neutrinoceros
Copy link
Contributor

Rationale

Numpy 1.25 (released yesterday) deprecates implicit conversions from 1-sized arrays to scalar types

While testing yt + cartopy, I discovered that such implicit conversion may be performed in pyproj.transformer.Transformer.transform, see https://github.com/pyproj4/pyproj/blob/32565ddf266658aebc9787b7534fdbdd06762839/pyproj/_transformer.pyx#L762

In order to avoid triggering deprecation warnings, the call site (here in cartopy) should handle this special case (pyproj cannot do it itself because it doesn't depend on numpy).

Implications

I assume there are already tests in cartopy that hit this condition, but properly testing this requires treating warnings as errors. I don't think this is done in cartopy yet, and it may require considerable efforts, way beyond the intended scope of this PR. Let me know what you guys think !

@greglucas
Copy link
Contributor

Is there a minimal reproducer with just Cartopy? When upgrading to numpy 1.25 and trying with scalars directly as below I don't get a deprecation warning.

In [1]: import cartopy.crs as ccrs

In [2]: import numpy as np

In [3]: p = ccrs.PlateCarree()

In [4]: p2 = ccrs.Mercator()

In [5]: p2.transform_point(np.float64(1), np.float64(2), p)

I'm not sure if this is best addressed in Cartopy or upstream in yt. i.e. could yt do this same conversion?

@neutrinoceros
Copy link
Contributor Author

In fact your example is already a minimal reproducer, you just need to specifically treat warnings as errors to see it. Wrapping your example in a script

# t.py
import cartopy.crs as ccrs
import numpy as np

p = ccrs.PlateCarree()
p2 = ccrs.Mercator()

p2.transform_point(np.float64(1), np.float64(2), p)

and invoking as python -Werror t.py will trigger the warning.

I'm not sure if this is best addressed in Cartopy or upstream in yt. i.e. could yt do this same conversion?

I think this is out of reach for yt because the problematic data (scalar arrays) seems to be generated from within cartopy. We're seeing an error (warning) specifically with the LambertAzimuthalEqualArea transform. There may be a better fix than my first proposed solution (maybe somewhere in cartopy.crs.CRS.transform_point(s)) but I haven't been able to find another yet.

@neutrinoceros
Copy link
Contributor Author

actually the reproducer can be reduced even further

# t.py
import cartopy.crs as ccrs

p = ccrs.PlateCarree()
p2 = ccrs.Mercator()

p2.transform_point(1, 2, p)

(the problem is with size 1 arrays, not actual numpy scalars, and the conversion from scalar to array is done internally)

@neutrinoceros neutrinoceros force-pushed the numpy1.25-scalars_safe_pj_transform branch from 0e17827 to f2da242 Compare June 21, 2023 08:10
@neutrinoceros
Copy link
Contributor Author

I've included the reprod as a test

@neutrinoceros neutrinoceros force-pushed the numpy1.25-scalars_safe_pj_transform branch from f2da242 to 0fb0ee8 Compare June 21, 2023 08:26
@greglucas
Copy link
Contributor

I actually think this should be updated/worked around in pyproj... I think just adding a filterwarnings to ignore this deprecation warning within pyproj would be sufficient. Note that it is within a try/except clause to try and go down a fast-path single-point route and if that doesn't work then falls back to a slower implementation. So we actually want the error, and now we are just getting an extra warning that I think can be safely ignored. (Although it should be checked whether it is still a TypeError that gets returned from numpy 2.0+

https://github.com/pyproj4/pyproj/blob/8f156a9d0eba49a746e772f95fbdebc19687ee7b/pyproj/transformer.py#L818-L830

@neutrinoceros
Copy link
Contributor Author

Sounds reasonable. I'll upstream the discussion to pyproj, and I'll come back to close this when I get things moving forward there, if that's okay with you.

@greglucas
Copy link
Contributor

Yes, that sounds totally reasonable :) feel free to ping me over there too since I am the one to blame for this "feature" in pyproj.

@neutrinoceros
Copy link
Contributor Author

I think the future error to be raised in NumPy 2.0 (or whenever the deprecation expires), is indeed TypeError, but for security I asked for confirmation in numpy/numpy#10615 (comment)

@neutrinoceros
Copy link
Contributor Author

The future is confirmed to be TypeError, so I think there's nothing to be done in pyproj and we can just live with a warning filter. My understanding is that cartopy doesn't even need such a filter at the moment, or does it ?

@neutrinoceros
Copy link
Contributor Author

@greglucas so do you think there's anything that needs be done here ?

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.

Yeah, I think we should do something about this, and practicality beats purity here I think. I have one further suggestion to get your thoughts on for just putting a warnings filter in rather than getting single items out.

else:
return a

return transformer.transform(
Copy link
Contributor

Choose a reason for hiding this comment

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

What about putting a context manager around a filterwarnings call here instead? The warning is sort of unfortunate and pyproj already has the workarounds for handling the single-item numpy arrays, so I guess I'd say we can just sweep the warning under the rug here because it is downstream and already taken care of.

warnings.filterwarnings("ignore", message="Conversion of an array with ndim > 0")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's try it

lib/cartopy/tests/crs/test_transform_point.py Outdated Show resolved Hide resolved
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.

Looks good to me. Thanks!

@greglucas greglucas merged commit 9b4a173 into SciTools:main Jun 23, 2023
@neutrinoceros neutrinoceros deleted the numpy1.25-scalars_safe_pj_transform branch June 23, 2023 21:06
@greglucas greglucas added this to the 0.22 milestone Jun 23, 2023
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.

2 participants