Skip to content

Fix for scalar detection #8821

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 6, 2025
Merged

Fix for scalar detection #8821

merged 8 commits into from
May 6, 2025

Conversation

huard
Copy link
Contributor

@huard huard commented Mar 11, 2024

_wrap_numpy_scalars relies on np.isscalar, which incorrectly labels a single cftime object as not a scalar.

import cftime
import numpy as np

c = cftime.datetime(2000, 1, 1, calendar='360_day') 
np.isscalar(c)  # False

The PR adds logic to handle non-numpy objects using the np.ndim function. The logic for built-ins and numpy objects should remain the same.

The function logic could possibly be rewritten more clearly as

    
    if hasattr(array, "dtype"):
        if np.isscalar(array):
            return np.array(array)
        else:
            return array
    
    if np.ndim(array) == 0:
        return np.array(array)
    
    return array

spencerkclark
spencerkclark reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@spencerkclark spencerkclark left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for digging into this @huard and @aulemahal. This looks reasonable to me based on the NumPy documentation you referenced, though I probably defer to others on whether there are any subtleties we need to worry about.

Is the inclusion of the hasattr(array, "dtype") condition to avoid casting zero-dimensional non-NumPy array types to NumPy arrays?

@huard
Copy link
Contributor Author

huard commented Mar 13, 2024 •
edited
Loading

It was rather to avoid casting 0-dimensional non-scalar numpy objects.

>>> a = np.array("abc")
>>> np.isscalar(a)
False
>>> np.ndim(a)
0

@dcherian dcherian requested a review from keewis March 15, 2024 04:45
keewis
keewis reviewed Apr 3, 2024
View reviewed changes
@Zeitsperre
Copy link
Contributor

Hi there, I have a library that could benefit from this fix, any updates here?

@dcherian dcherian requested a review from keewis November 4, 2024 21:10
keewis
keewis reviewed Nov 4, 2024
View reviewed changes
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 ping, @dcherian, and apologies for saying I would look into this and then promptly forgetting about it. I believe it was good to wait until after numpy=2.1, though, since that appears to have broken is_duck_array.

@Zeitsperre Zeitsperre mentioned this pull request Feb 26, 2025
Merged
4 tasks
aulemahal and others added 3 commits March 3, 2025 11:29
@aulemahal
Merge branch 'main' into fix-7794
4379f29
@aulemahal @keewis
Update xarray/core/indexing.py
6e0e9e9 
Co-authored-by: Justus Magin <keewis@users.noreply.github.com>
@aulemahal
run precommit
6c4bdf2
@aulemahal
Copy link
Contributor

@keewis I applied your suggestion and it seems the issue is fixed! Failures in the tests do not seem related to this PR, I see that the test_where issue is also triggered by the latest nightly upstream CI run.

@Zeitsperre Zeitsperre mentioned this pull request Mar 10, 2025
Merged
4 tasks
@keewis keewis closed this Apr 17, 2025
@keewis keewis reopened this Apr 17, 2025
@keewis
Copy link
Collaborator

keewis commented Apr 17, 2025

sorry for missing this a month ago. Could you merge in main, please? That should resolve most failing tests (except mypy, apparently, but that's also failing on main)

@Zeitsperre
Copy link
Contributor

@keewis Done. Thanks again!

@dcherian dcherian changed the title Add small test exposing issue from #7794 and suggestion for _wrap_numpy_scalars fix Fix for scalar detection May 6, 2025
@dcherian dcherian merged commit 4be6ca4 into pydata:main May 6, 2025
32 checks passed
dcherian added a commit to dcherian/xarray that referenced this pull request May 7, 2025
@dcherian
Merge branch 'main' into fix-getattr-extension
7e35116 
* main:
  dev whats-new (pydata#10294)
  Add SeasonGrouper, SeasonResampler (pydata#9524)
  (fix): remove `PandasExtensionArray` from repr (pydata#10291)
  Do not rely on `np.broadcast_to` to perform trivial dimension insertion (pydata#10277)
  DOC: Remove reference to absolufy (pydata#10290)
  Fix for scalar detection (pydata#8821)
  Add Index.validate_dataarray_coord (pydata#10137)
  add redirect for contributing guide (pydata#10282)
  Update pre-commit hooks (pydata#10288)
  Adding xarray-eopf to ecosystem.rst (pydata#10289)
  Add public typing.py module (pydata#10215)
  Rename Twitter to X (pydata#10283)
  Add `xarray-lmfit` extension for curve fitting to ecosystem documentation (pydata#10262)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian left review comments

@spencerkclark spencerkclark spencerkclark left review comments

@keewis keewis keewis left review comments

Assignees
No one assigned
Labels
topic-documentation topic-indexing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeError for time_bnds variable when calling Dataset.to_netcdf
6 participants
@huard @Zeitsperre @aulemahal @keewis @dcherian @spencerkclark