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

Add small test exposing issue from #7794 and suggestion for _wrap_numpy_scalars fix #8821

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

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

Copy link
Member

@spencerkclark spencerkclark 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 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

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
@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
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.

@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.

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.

TypeError for time_bnds variable when calling Dataset.to_netcdf
6 participants