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

ENH: size() to return None on dask instead of nan #231

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

crusaderky
Copy link
Contributor

@crusaderky crusaderky commented Jan 7, 2025

dask arrays of unknown shape return NaN in their .size and .shape properties. This was chosen a long time before the Array API standard existed, but alas diverges from the array API standard itself:

ndonnx, another library which can have arrays of unknown shape, uses None instead.

This PR changes the size() function to make dask conform to the standard and adds units tests which were previously missing.

Q: Should we add a shape() function to cover for Dask's .shape property non-standard behaviour?

@crusaderky crusaderky mentioned this pull request Jan 7, 2025
@crusaderky crusaderky force-pushed the test_size branch 3 times, most recently from af0a5db to d9f5293 Compare January 8, 2025 10:34
assert size(x) == 3


@pytest.mark.parametrize("library", all_libraries)
Copy link
Contributor Author

@crusaderky crusaderky Jan 8, 2025

Choose a reason for hiding this comment

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

This doesn't actually test ndonnx's None case until #232 gets merged.

@crusaderky
Copy link
Contributor Author

CI failure seems unrelated

Copy link
Contributor

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @crusaderky

@ev-br ev-br enabled auto-merge January 9, 2025 06:38
@ev-br ev-br merged commit e5dd419 into data-apis:main Jan 9, 2025
83 of 84 checks passed
@crusaderky crusaderky deleted the test_size branch January 11, 2025 22:36
crusaderky added a commit to crusaderky/array-api-compat that referenced this pull request Jan 14, 2025
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