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

Fix a regression with scalar indexing due to #1800 #1875

Merged
merged 9 commits into from
May 16, 2024

Conversation

dcherian
Copy link

@dcherian dcherian commented May 13, 2024

Fixes a regression in indexing out scalars from size-1 chunks.

TODO:

Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.98%. Comparing base (8264ace) to head (da2dd72).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1875   +/-   ##
=======================================
  Coverage   99.98%   99.98%           
=======================================
  Files          38       38           
  Lines       14667    14695   +28     
=======================================
+ Hits        14665    14693   +28     
  Misses          2        2           
Files Coverage Δ
zarr/core.py 100.00% <100.00%> (ø)
zarr/indexing.py 100.00% <100.00%> (ø)
zarr/tests/test_core.py 99.94% <100.00%> (+<0.01%) ⬆️

Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks Deepak!

@dcherian dcherian marked this pull request as draft May 14, 2024 14:59
@@ -52,6 +52,8 @@ def is_scalar(value, dtype):
return True
if isinstance(value, tuple) and dtype.names and len(value) == len(dtype.names):
return True
if dtype.kind == "O" and not isinstance(value, np.ndarray):
Copy link
Author

@dcherian dcherian May 16, 2024

Choose a reason for hiding this comment

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

This is the core issue in #1874. We need to treat [1, 'C'] as a scalar for object arrays.

Is this the right condition?

cc @jakirkham @aplowman

@dcherian dcherian marked this pull request as ready for review May 16, 2024 15:29
@jhamman
Copy link
Member

jhamman commented May 16, 2024

@zarr-developers/python-core-devs - I'm planning to merge this and make a 2.18.1 release later today.

Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

Thanks Deepak!

@jhamman jhamman merged commit 908e920 into zarr-developers:main May 16, 2024
18 checks passed
@jakirkham
Copy link
Member

Thanks all! 🙏

@dcherian dcherian deleted the fix-scalar-indexing-regression branch June 11, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants