Encapsulate Python sequence-like indexers

jakelishman commented 5 days ago

Summary

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.

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 thiserror as a crate for generating the boilerplate for implementing Error for return values. I barely used it in this PR, but I split this off from a follow-up defining SparseObservable, which uses it more heavily.

qiskit-bot commented 5 days ago

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@kevinhartman
@levbishop
@mtreinish

coveralls commented 5 days ago

Pull Request Test Coverage Report for Build 9681825290

Details

194 of 240 (80.83%) changed or added relevant lines in 4 files are covered.
16 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.02%) to 89.768%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/euler_one_qubit_decomposer.rs	3	9	33.33%
crates/accelerate/src/two_qubit_decompose.rs	3	9	33.33%
crates/circuit/src/circuit_data.rs	82	93	88.17%
crates/circuit/src/slice.rs	106	129	82.17%
<!--	Total:	194	240	80.83%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	91.86%
crates/qasm2/src/parse.rs	12	96.23%
<!--	Total:	16	-->

Totals
Change from base Build 9680727079:	0.02%
Covered Lines:	63806
Relevant Lines:	71079

💛 - Coveralls

coveralls commented 4 days ago

Pull Request Test Coverage Report for Build 9686831288

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

192 of 236 (81.36%) changed or added relevant lines in 4 files are covered.
93 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.06%) to 89.806%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/euler_one_qubit_decomposer.rs	3	9	33.33%
crates/accelerate/src/two_qubit_decompose.rs	3	9	33.33%
crates/circuit/src/circuit_data.rs	82	93	88.17%
crates/circuit/src/slice.rs	104	125	83.2%
<!--	Total:	192	236	81.36%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/lex.rs	4	92.88%
qiskit/circuit/quantumcircuit.py	16	95.69%
crates/circuit/src/operations.rs	72	72.35%
<!--	Total:	93	-->

Totals
Change from base Build 9680727079:	0.06%
Covered Lines:	63843
Relevant Lines:	71090

💛 - Coveralls

coveralls commented 3 days ago

Pull Request Test Coverage Report for Build 9715953948

Details

192 of 236 (81.36%) changed or added relevant lines in 4 files are covered.
15 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.04%) to 89.783%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/euler_one_qubit_decomposer.rs	3	9	33.33%
crates/accelerate/src/two_qubit_decompose.rs	3	9	33.33%
crates/circuit/src/circuit_data.rs	82	93	88.17%
crates/circuit/src/slice.rs	104	125	83.2%
<!--	Total:	192	236	81.36%	-->

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	3	92.62%
crates/qasm2/src/parse.rs	12	97.15%
<!--	Total:	15	-->

Totals
Change from base Build 9714383578:	0.04%
Covered Lines:	63840
Relevant Lines:	71105

💛 - Coveralls

jakelishman commented 5 hours ago

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 Range that could be passed through the various iterator methods. I've mostly just added the bits we need for Qiskit immediately (though tbh DoubleEndedIterator for Descending was probably not 100% required) to avoid further bloating a PR that was already more involved than I was expecting.

Qiskit / qiskit