Skip to content
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

Bump PyO3 and rust-numpy to 0.23.x #13577

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

mtreinish
Copy link
Member

Summary

This commit bumps the version of pyo3 and rust-numpy used by qiskit to the latest release 0.23. The largest change by volume of code is the deprecation of all the *_bound() methods. These are just warnings but they would be fatal to our CI so it needs to be updated. The larger functional change that required updating the code is the change in the traits around converting to Python objects. This actually found a bug in the target where we were not returning a proper instruction type for standard gates. This also opens up the opportunity to update our hashbrown version to 0.15.x, but we can't do that until the next rustworkx-core release.

Details and comments

This commit bumps the version of pyo3 and rust-numpy used by qiskit to
the latest release 0.23. The largest change by volume of code is the
deprecation of all the `*_bound()` methods. These are just warnings but
they would be fatal to our CI so it needs to be updated. THe larger
functional change that required updating the code is the change in the
traits around converting to Python objects. This actually found a bug in
the target where we were not returning a proper instruction type for
standard gates. This also opens up the opportunity to update our
hashbrown version to 0.15.x, but we can't do that until the next
rustworkx-core release.
@mtreinish mtreinish added Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Dec 18, 2024
@mtreinish mtreinish added this to the 2.0.0 milestone Dec 18, 2024
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core
  • @levbishop

@kevinhartman kevinhartman self-assigned this Dec 18, 2024
Copy link
Contributor

@kevinhartman kevinhartman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! I've done just one quick pass for now, and will unassign myself since I'm unlikely to have another look before I'm out on vacation.

.get_bound(py)
.call1((array.to_object(py), py.None(), false))?;
let unitary_gate = UNITARY_GATE.get_bound(py).call1((
<pyo3::Bound<'_, PyArray<Complex<f64>, ndarray::Dim<[usize; 2]>>> as Clone>::clone(&array).into_pyobject(py)?,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

crates/accelerate/src/sabre/heuristic.rs Outdated Show resolved Hide resolved
crates/accelerate/src/target_transpiler/mod.rs Outdated Show resolved Hide resolved
crates/circuit/src/operations.rs Outdated Show resolved Hide resolved
@kevinhartman kevinhartman removed their assignment Dec 19, 2024
In pyo3 0.23 the behavior of the default conversion for a `SmallVec<u8>`
and similar arrays of u8 default to bytes objects when converted to
python. This was documented as an api change in the release, however it
was cauing the unitary synthesis test to fail because when we were
evaluating whether the synthesis was in terms of the natural direction
of the 2q gate on the backend it was evaluating `[0, 1] == b"\x00\x11"`
which evaluates to `False` and the pass flipped the direction of the
2q gate. This was causing the test failure because all the 2q gates
which were correctly directed were getting incorrectly flipped. This
commit fixes this by manually creating a pylist so the comparisons work
as expected.
@coveralls
Copy link

coveralls commented Jan 9, 2025

Pull Request Test Coverage Report for Build 12698031086

Details

  • 878 of 1009 (87.02%) changed or added relevant lines in 58 files are covered.
  • 35 unchanged lines in 16 files lost coverage.
  • Overall coverage decreased (-0.02%) to 88.916%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/accelerate/src/consolidate_blocks.rs 7 8 87.5%
crates/accelerate/src/sparse_pauli_op.rs 18 19 94.74%
crates/accelerate/src/synthesis/linear/mod.rs 7 8 87.5%
crates/circuit/src/dag_node.rs 19 20 95.0%
crates/circuit/src/lib.rs 1 2 50.0%
crates/qasm3/src/build.rs 7 8 87.5%
crates/accelerate/src/commutation_analysis.rs 7 9 77.78%
crates/accelerate/src/results/marginalization.rs 6 8 75.0%
crates/accelerate/src/utils.rs 0 3 0.0%
crates/qasm3/src/lib.rs 11 15 73.33%
Files with Coverage Reduction New Missed Lines %
crates/circuit/src/converters.rs 1 95.65%
crates/circuit/src/circuit_instruction.rs 1 75.93%
crates/accelerate/src/gate_direction.rs 1 97.12%
crates/accelerate/src/sparse_observable.rs 1 95.06%
crates/accelerate/src/commutation_checker.rs 1 97.23%
crates/accelerate/src/commutation_analysis.rs 1 95.28%
crates/qasm3/src/circuit.rs 1 78.48%
crates/accelerate/src/utils.rs 1 40.91%
crates/accelerate/src/basis/basis_translator/mod.rs 1 87.44%
crates/qasm2/src/lex.rs 2 92.48%
Totals Coverage Status
Change from base Build 12687984222: -0.02%
Covered Lines: 79471
Relevant Lines: 89378

💛 - Coveralls

@jakelishman jakelishman self-assigned this Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants