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
DM-10271: Support deleting slices of a catalog. #222
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.
Some things to fix before merging.
throw py::error_already_set(); | ||
} | ||
if (step != 1) { | ||
throw py::index_error("Slice step must not be negative"); |
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.
step != 1
does not necessarily mean it's negative.
self.assertEqual([r.get(key) for r in catalog], | ||
[0, 1, 2, 3, 8, 9]) | ||
with self.assertRaises(IndexError): | ||
del catalog[1:3:-1] |
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.
Check other steps beside -1
.
tests/testSimpleTable.py
Outdated
with self.assertRaises(IndexError): | ||
del catalog[1:3:-1] | ||
with self.assertRaises(IndexError): | ||
del catalog[50] |
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.
Well, that's rather inconvenient and counter-intuitive: I can delete a slice but not a single element! I think you need to support this.
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.
You can delete a single element, just not one with index >= len(catalog)
.
f119141
to
6c63c9f
Compare
Helpful in the peak-transferring code needed on the meas_algorithms branch for this issue.