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

Change np.core.defchararray to np.char (#9165) #9166

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

pont-us
Copy link
Contributor

@pont-us pont-us commented Jun 24, 2024

Replace a reference to np.core.defchararray with np.char.chararray in xarray.testing.assertions, since the former no longer works on NumPy 2.0.0 and the latter is the "preferred alias" according to NumPy docs. See Issue #9165.

Replace a reference to np.core.defchararray with np.char.chararray
in xarray.testing.assertions, since the former no longer works on
NumPy 2.0.0 and the latter is the "preferred alias" according to
NumPy docs. See Issue pydata#9165.
Copy link

welcome bot commented Jun 24, 2024

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, @pont-us.

Could you add a test to make sure we don't regress, please? This could be as simple as adding another parameter set to

@pytest.mark.parametrize(
"obj1,obj2",
(
pytest.param(
xr.Variable("x", [1e-17, 2]), xr.Variable("x", [0, 3]), id="Variable"
),
pytest.param(
xr.DataArray([1e-17, 2], dims="x"),
xr.DataArray([0, 3], dims="x"),
id="DataArray",
),
pytest.param(
xr.Dataset({"a": ("x", [1e-17, 2]), "b": ("y", [-2e-18, 2])}),
xr.Dataset({"a": ("x", [0, 2]), "b": ("y", [0, 1])}),
id="Dataset",
),
),
)
def test_assert_allclose(obj1, obj2) -> None:
with pytest.raises(AssertionError):
xr.testing.assert_allclose(obj1, obj2)
with pytest.raises(AssertionError):
xr.testing.assert_allclose(obj1, obj2, check_dim_order=False)
or
def test_allclose_regression() -> None:
x = xr.DataArray(1.01)
y = xr.DataArray(1.02)
xr.testing.assert_allclose(x, y, atol=0.01)

You might also want to add a note to whats-new.rst

xarray/testing/assertions.py Outdated Show resolved Hide resolved
@pont-us
Copy link
Contributor Author

pont-us commented Jun 25, 2024

You might also want to add a note to whats-new.rst

I didn't think this would qualify as a "notable" bug fix, but glad to oblige :)

@pont-us pont-us marked this pull request as ready for review June 25, 2024 16:37
@pont-us pont-us changed the title Change np.core.defchararray to np.char.chararray (#9165) Change np.core.defchararray to np.char (#9165) Jun 25, 2024
Copy link
Collaborator

@keewis keewis 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, just one small nit.

xarray/tests/test_assertions.py Outdated Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
@pont-us
Copy link
Contributor Author

pont-us commented Jun 26, 2024

From a quick look, it appears that the failing Doctests are due to other, unrelated NumPy 2 changes (mainly NEP 51).

@keewis
Copy link
Collaborator

keewis commented Jun 26, 2024

yep, this is also fails on main, so you can ignore these.

@keewis keewis added the plan to merge Final call for comments label Jun 26, 2024
doc/whats-new.rst Outdated Show resolved Hide resolved
@max-sixty max-sixty merged commit 651bd12 into pydata:main Jun 26, 2024
16 of 28 checks passed
Copy link

welcome bot commented Jun 26, 2024

Congratulations on completing your first pull request! Welcome to Xarray! We are proud of you, and hope to see you again! celebration gif

@max-sixty
Copy link
Collaborator

Thanks @pont-us

(any other changes for numpy 2.0 would be greatly appreciated if you're up for it...)

@keewis
Copy link
Collaborator

keewis commented Jun 26, 2024

just FYI, the doctest / rtd will be fixed by #9177, and the normal CI will be fixed by #9136 (we just need to figure out whether using view is a good idea). This leaves us with the pydap plugin test, but since pydap raises an error with numpy>=2 on import there's nothing we can do about this other than disabling in CI and raising an issue on the pydap issue tracker.

@max-sixty
Copy link
Collaborator

That's awesome, thanks @keewis !

@pont-us
Copy link
Contributor Author

pont-us commented Jun 26, 2024

Thanks for the help @keewis and thanks for the thanks @max-sixty :). I don't think I'll have time for another PR in the short term, unfortunately, but I plan to at least open issues for one or two other NumPy-2-related surprises that I've come across (if nobody else does first).

dcherian added a commit to dcherian/xarray that referenced this pull request Jun 28, 2024
* main:
  promote floating-point numeric datetimes to 64-bit before decoding (pydata#9182)
  also pin `numpy` in the all-but-dask CI (pydata#9184)
  temporarily remove `pydap` from CI (pydata#9183)
  temporarily pin `numpy<2` (pydata#9181)
  Change np.core.defchararray to np.char (pydata#9165) (pydata#9166)
  Fix example code formatting for CachingFileManager (pydata#9178)
  Slightly improve DataTree repr (pydata#9064)
  switch to unit `"D"` (pydata#9170)
  Docs: Add page with figure for navigating help resources (pydata#9147)
  Add test for pydata#9155 (pydata#9161)
  Remove mypy exclusions for a couple more libraries (pydata#9160)
  Include numbagg in type checks (pydata#9159)
  Improve zarr chunks docs (pydata#9140)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testing.assert_allclose on string dtype gives error with NumPy 2.0.0
3 participants