-
-
Notifications
You must be signed in to change notification settings - Fork 348
Fix incorrect Group.nmembers for consolidated metadata #3287
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 incorrect Group.nmembers for consolidated metadata #3287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks. Could you add a release note (https://zarr.readthedocs.io/en/stable/developers/contributing.html#changelog)?
Can you also ensure that we have test coverage for the case where we raise a ValueError
?
with pytest.raises(ValueError, match="max_depth"):
await group.nmembers(max_depth=-1)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3287 +/- ##
==========================================
+ Coverage 59.65% 59.67% +0.01%
==========================================
Files 78 78
Lines 8690 8694 +4
==========================================
+ Hits 5184 5188 +4
Misses 3506 3506
🚀 New features to boost your workflow:
|
Done. Let me know if there's anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Addresses the issue where
Group.nmembers()
doesn't honor themax_depth
when the group has consolidated metadata. This simply filtersflattened_metadata
according to the number of/
in the sub-elements' paths, which is analogous to checking the depth.I'm new to this project, so I'm not sure if the implemented test is sufficient.
Closes #3252
TODO:
docs/user-guide/*.rst
changes/