Allow negative slices when indexing chunked data#170
Allow negative slices when indexing chunked data#170valeriupredoi merged 13 commits intoNCAS-CMS:mainfrom
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (93.02%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
==========================================
+ Coverage 76.83% 77.33% +0.49%
==========================================
Files 15 15
Lines 2936 2974 +38
Branches 467 477 +10
==========================================
+ Hits 2256 2300 +44
+ Misses 558 555 -3
+ Partials 122 119 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bnlawrence
left a comment
bnlawrence
left a comment
There was a problem hiding this comment.
This looks pretty straightforward I think, but I just have some minor comment comments to consider.
pyfive/indexing.py
Outdated
| def __init__(self): | ||
| super().__init__("only slices with step >= 1 are supported") | ||
|
|
||
| # And the rest of the code is the original file. |
There was a problem hiding this comment.
I guess this is no longer true
There was a problem hiding this comment.
That's right. I think that negative step slices are OK in all cases, now ...
There was a problem hiding this comment.
So I would remove that comment (line 50), then I think this can be approved.
pyfive/indexing.py
Outdated
| elif index == Ellipsis: | ||
| raise ValueError( | ||
| "replace_negative_slices doesn't work when selection " | ||
| "contains Ellipsis. Consider running replace_ellipsis first" |
There was a problem hiding this comment.
What is "replace_ellipsis", would the user know about this, could it have a link?
There was a problem hiding this comment.
I've changed the message to
"replace_negative_slices(selection, shape) doesn't work "
"when selection contains Ellipsis. Consider doing "
"selection=pyfive.indexing.replace_ellipsis(selection, shape) "
"first"
pyfive/indexing.py
Outdated
| # dimensions might get dropped by integer indices) | ||
| dim = 0 | ||
|
|
||
| # Parse the indices |
There was a problem hiding this comment.
Maybe it's not so much parse the indices, as "parse and replace the indices"?
bnlawrence
left a comment
bnlawrence
left a comment
There was a problem hiding this comment.
I'm good to go once line 50 is gone (I would have done it myself .. .but)
I'm perfectly happy to do this, but was just wondering why, as that line has "always" been there ... |
|
OK - all suggested changes done. Good to go? |
|
Tests now passing (just needed a |
wunderschnitzel! I'll approve and merge, many thanks @davidhassell and @bnlawrence 🍻 |
3 participants
Description
Currently (v1.0.1) negative slices, i.e. slices with a negative step, such as
[10:1:-1], are not allowed on chunked data. This breaks some downstream applications (e.g. cfdm and cf-python). This can be fixed.Closes #169
Before you get started
Checklist