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.

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

More extensive orthogonal indexing in get/setitem #1333

Merged
merged 3 commits into from
Mar 10, 2023
Merged

More extensive orthogonal indexing in get/setitem #1333

merged 3 commits into from
Mar 10, 2023

Conversation

AndreasAlbertQC
Copy link
Contributor

@AndreasAlbertQC AndreasAlbertQC commented Jan 25, 2023 •
edited
Loading

Addresses #1029

Added getitem/setitem pass-through to orthogonal indexing for the following cases:

  • index is iterable of integers
  • index is iterable of length ndim, with each element being a slice, integer, or list. Maximum one list.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@jakirkham
Copy link
Member

cc @jni (in case this is of interest 🙂)

@codecov
Copy link

codecov bot commented Jan 25, 2023 •
edited
Loading

Codecov Report

Merging #1333 (64489cd) into main (c77f9cd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1333   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines        14775     14802   +27     
=========================================
+ Hits         14775     14802   +27
Impacted Files Coverage Δ
zarr/core.py 100.00% <100.00%> (ø)
zarr/indexing.py 100.00% <100.00%> (ø)
zarr/tests/test_indexing.py 100.00% <100.00%> (ø)

@AndreasAlbertQC
More extensive orthogonal indexing in get/setitem
1667199 
Added pass-through to orthogonal indexing for the following cases:
* index is iterable of integers
* index is iterable of length ndim, with each element being a slice, integer, or list. Maximum one list.
@AndreasAlbertQC AndreasAlbertQC marked this pull request as ready for review January 31, 2023 09:29
@joshmoore
Copy link
Member

Agreed, I think @jni will be happy to see someone taking up the mantle!

@AndreasAlbertQC
Copy link
Contributor Author

Thanks @jakirkham , @joshmoore. Please let me know if anything else is needed from my side. I figure I will resolve the conflicts after code review, so I can do it in one go rather than piecemeal.

jni
jni approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@jni jni left a comment

Choose a reason for hiding this comment

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

Brilliant stuff @AndreasAlbertQC! I have to admit I don't know the numpy rules in enough detail to properly grok all the case 2 conditions, but the tests test all of the questions I might have thoroughly so I'm happy with this. Major usability win. 😊

@joshmoore
Copy link
Member

joshmoore commented Mar 10, 2023 •
edited
Loading

Thanks, @jni! Fixed the releases.rst conflict. Will start prepping for 2.15.x.

@jni jni merged commit eacda8d into zarr-developers:main Mar 10, 2023
@AndreasAlbertQC
Copy link
Contributor Author

Thank you!

@MSanKeys963 MSanKeys963 mentioned this pull request Mar 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jni jni jni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AndreasAlbertQC @jakirkham @joshmoore @jni