Encapsulate Python sequence-like indexers #12669
Conversation
jakelishman
added
type: qa
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 9681825290Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9686831288Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| let index = *index as usize; | ||
| if index >= len { |
There was a problem hiding this comment.
Could this also be written in a single line as follows? 🙂
| let index = *index as usize; | |
| if index >= len { | |
| if *index as usize >= len { |
There was a problem hiding this comment.
I don't remember exactly why I split it off like this, but if you want to change it, line 54 would also need changing to the same thing. If you want to remake the suggestion with both I'll commit it.
| gap / *step + (gap % *step != 0) as usize | ||
| } | ||
| Self::NegRange { start, stop, step } => 'arm: { | ||
| let Some(start) = start else { break 'arm 0 }; |
There was a problem hiding this comment.
Now I'm relieved we don't share an office with you breaking limbs 😉 but on a more serious note, I'm a bit confused about this, couldn't I have a Python slice with negative start and stop and then this would end up with two None values?
There was a problem hiding this comment.
Haha yeah, I meant the "match arm" but I didn't even think about that!
Yes, you can have two None, and in fact I think that's the only time indices.start can be None. Either if it weren't, the length would still always be zero because the start point isn't a valid index.
This encapsulates a lot of the common logic around Python sequence-like indexers (`SliceOrInt`) into iterators that handle adapting negative indices and slices in `usize` for containers of a given size. These indexers now all implement `ExactSizeIterator` and `DoubleEndedIterator`, so they can be used with all `Iterator` methods, and can be used (with `Iterator::map` and friends) as inputs to `PyList::new_bound`, which makes code simpler at all points of use. The special-cased uses of this kind of thing from `CircuitData` are replaced with the new forms. This had no measurable impact on performance on my machine, and removes a lot noise from error-handling and highly specialised functions.
jakelishman
force-pushed
the
sequence-index
branch
from
6083896 to
dc20c20
Compare
Pull Request Test Coverage Report for Build 9715953948Details
💛 - Coveralls |
There was a problem hiding this comment.
This lgtm, this is a great abstraction to add and makes working with slices much less error prone. All the logic seems super through and accounted for edge cases I didn't even realize were there. As a standalone PR I think this is good to merge.
But I'm wondering about rustworkx and probably any other pyo3 users out there. I'm thinking we should contribute this to PyO3 or have it somewhere more publicly accessible because I think it'll be useful for any custom sequence types people are creating with PyO3. It's providing a nice API to work with slices which are super error prone with the current pyo3 interface because they're super low level and the Python C API doesn't document things super well.
| let py = value.py(); | ||
| match index.with_len(self.data.len())? { | ||
| SequenceIndex::Int(index) => set_single(self, index, value), | ||
| indices @ SequenceIndex::PosRange { |
There was a problem hiding this comment.
I think this is the first time I've seen the match binding syntax used in practice.
There was a problem hiding this comment.
I think I used it in the OQ2 lexer as well - I don't use it often, but it's occasionally useful.
| fn pack(&mut self, inst: PyRef<CircuitInstruction>) -> PyResult<PackedInstruction> { | ||
| let py = inst.py(); |
There was a problem hiding this comment.
This feels a bit unnecessary but it's not a big deal I guess.
There was a problem hiding this comment.
Yeah, I don't think I intended to make a change here - it might be a remnant of something else I was up to.
| // `slice` can't be subclassed in Python, so it's safe (and faster) to check for it exactly. | ||
| // The `downcast_exact` check is just a pointer comparison, so while `slice` is the less | ||
| // common input, doing that first has little-to-no impact on the speed of the `isize` path, | ||
| // while the reverse makes `slice` inputs significantly slower. | ||
| if let Ok(slice) = ob.downcast_exact::<PySlice>() { | ||
| return Ok(Self::Slice(slice.clone())); | ||
| } | ||
| Ok(Self::Int(ob.extract()?)) |
There was a problem hiding this comment.
Heh, this makes me think of: Qiskit/rustworkx#1096 where you opted for just flipping the order. This is starting to make me think maybe we want to contribute this to PyO3 so there is a common impl of this for everyone.
There was a problem hiding this comment.
You're just watching me learning more of the low-level internals of everything live haha.
In Rustworkx I didn't know enough other than to leave the FromPyObject derivation in place, in which case flipping the order just makes things faster. But for the sake of completeness here, since I'd already wasted enough time that a little longer made no difference, I actually checked whether slice is subclassable (turns out it isn't), in which case the derived FromPyObject is needlessly slow; you can't subclass it, so the type is slice check is sufficient and is just a pointer comparison.
|
Yeah, I considered talking to PyO3 about contributing it upstream, though I'm thinking maybe we play with it a shade longer within Qiskit first to see if the abstraction actually bears weight. I'm ok with the interface I've come up with here, but I think it could still be better. If nothing else, there's tons of specialisations of |
This encapsulates a lot of the common logic around Python sequence-like indexers (`SliceOrInt`) into iterators that handle adapting negative indices and slices in `usize` for containers of a given size. These indexers now all implement `ExactSizeIterator` and `DoubleEndedIterator`, so they can be used with all `Iterator` methods, and can be used (with `Iterator::map` and friends) as inputs to `PyList::new_bound`, which makes code simpler at all points of use. The special-cased uses of this kind of thing from `CircuitData` are replaced with the new forms. This had no measurable impact on performance on my machine, and removes a lot noise from error-handling and highly specialised functions.
Summary
This encapsulates a lot of the common logic around Python sequence-like indexers (
SliceOrInt) into iterators that handle adapting negative indices and slices inusizefor containers of a given size.These indexers now all implement
ExactSizeIteratorandDoubleEndedIterator, so they can be used with allIteratormethods, and can be used (withIterator::mapand friends) as inputs toPyList::new_bound, which makes code simpler at all points of use.The special-cased uses of this kind of thing from
CircuitDataare replaced with the new forms. This had no measurable impact on performance on my machine, and removes a lot noise from error-handling and highly specialised functions.Details and comments
This PR was not meant to be this big - I only started writing something up because I needed similar logic to things we already had in 3+ different places for
SparseObservable, and then I let it get a bit more out of hand.This introduces
thiserroras a crate for generating the boilerplate for implementingErrorfor return values. I barely used it in this PR, but I split this off from a follow-up definingSparseObservable, which uses it more heavily.