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

use numpy >=1.20 #70

Merged
merged 9 commits into from
Mar 13, 2023
Merged

use numpy >=1.20 #70

merged 9 commits into from
Mar 13, 2023

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Mar 9, 2023

The np.object, and many other aliases, as deprecated and removed in numpy 1.24. This updates the code and set a new min numpy version.

@kwilcox I'd appreciate if we can merge this quick and issue a new release. I can take care of the latter.

@ocefpaf ocefpaf requested a review from kwilcox March 9, 2023 13:28
@ocefpaf ocefpaf mentioned this pull request Mar 9, 2023
@ocefpaf ocefpaf marked this pull request as draft March 9, 2023 13:47
@ocefpaf ocefpaf marked this pull request as ready for review March 9, 2023 17:32
with:
python-version: 3.x
python-version: "3.10"
Copy link
Member Author

Choose a reason for hiding this comment

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

we cannot use latest Python until we resolve the netcdf4<1.6.1 pin.

@@ -133,7 +133,7 @@ def from_dataframe(cls, df, output, **kwargs):
profile[j] = pname
row_size[j] = len(pfg)
if s_ind is not None:
s_ind[j] = np.asscalar(np.argwhere(station[:] == pfg[axes.station].dropna().iloc[0]))
Copy link
Member Author

Choose a reason for hiding this comment

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

Numpy depreaction fix.

@@ -72,6 +72,9 @@ def get_geographic_attributes(df, axes=None):

if len(set(coords)) == 1:
geoclass = Point
# The set is to workaround the fact tht pocean
# relied in a shapely<2 bug to pass a vector here instead of a point.
coords = set(coords)
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if you want to leave this as-is or to re-write it but it was relying on a bad behavior before. One cannot pass 4 coords to a Point class.

Copy link
Member

Choose a reason for hiding this comment

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

From the Shapely 2.0 release notes:

The Point(..) constructor no longer accepts a sequence of coordinates consisting of more than one coordinate pair (previously, subsequent coordinates were ignored) (#1600).

@@ -274,7 +274,7 @@ def create_ncvar_from_series(ncd, var_name, dimensions, series, **kwargs):
elif series.dtype.kind in ['U', 'S'] or series.dtype in [str]:
# AttributeError: cannot set _FillValue attribute for VLEN or compound variable
v = ncd.createVariable(var_name, get_dtype(series), dimensions, **kwargs)
elif series.dtype == np.object:
elif series.dtype == object:
Copy link
Member Author

Choose a reason for hiding this comment

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

Numpy deprecation.

@@ -29,12 +29,12 @@ classifiers =
packages = find:
install_requires =
cftime>=1.2.1
netcdf4
numpy>=1.15
netcdf4<1.6.1
Copy link
Member Author

@ocefpaf ocefpaf Mar 9, 2023

Choose a reason for hiding this comment

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

This pin will hurt us when trying to install pocean in modern envs.
I was unable to fix the failing tests without it. Iris wrote a whole new class to solve that. I was hoping for an easier solution here :-/

@ocefpaf
Copy link
Member Author

ocefpaf commented Mar 13, 2023

@kwilcox with the netcdf4<1.6.1, pin and the deprecation fixes, everything passes now. However, I don't like that pin b/c it holds us back from upgrading envs.

I could not see a clear path ahead to easily fix this in pocean-core. For more context on the issue see:

Unidata/netcdf4-python#1192
SciTools/iris#5095
Unidata/netcdf4-python#1193

For now I'm OK with this solution and it kind of helps us already. It would be nice to get someone more familiar with pocean-core to tackle the netcdf4 pin though.

@kwilcox
Copy link
Member

kwilcox commented Mar 13, 2023

Thanks @ocefpaf! I agree, solving the netcdf4 pin is out of scope for this PR (and a pain). Just a note, I wouldn't be against pinning shapely>1.8 in the future to resolve this years old workaround.

@kwilcox kwilcox merged commit 39a3767 into pyoceans:main Mar 13, 2023
@ocefpaf ocefpaf deleted the update_numpy branch March 13, 2023 13:02
@ocefpaf
Copy link
Member Author

ocefpaf commented Mar 13, 2023

Just a note, I wouldn't be against pinning shapely>1.8 in the future to resolve this years old workaround.

I can look into that now. If I manage to fix this today maybe we can even issue a new release in the afternoon.

@ocefpaf ocefpaf mentioned this pull request Mar 13, 2023
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.

2 participants