Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
4.83k stars 2.29k forks source link

Add test case to validate the rust->Python gate conversion #12623

Closed mtreinish closed 1 week ago

mtreinish commented 1 week ago

Summary

This commit adds a test to the test_rust_equivalence module to assert that the Python gate objects returned from the Rust CircuitData is the correct type.

Details and comments

qiskit-bot commented 1 week ago

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

coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 9604586227

Details


Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 91.86%
crates/qasm2/src/parse.rs 6 97.61%
<!-- Total: 10 -->
Totals Coverage Status
Change from base Build 9602245093: 0.0%
Covered Lines: 63573
Relevant Lines: 70841

💛 - Coveralls
mtreinish commented 1 week ago

There is a path where those imports don't get executed: https://github.com/Qiskit/qiskit/blob/main/crates/circuit/src/circuit_instruction.rs#L743-L785 If a TGate python object is added to any circuit directly then we get the mapping automatically from that for all future uses of StandardGate::TGate from the class attribute. Those import paths are just a fallback that gets executed if we don't go through python on the construction side and we need to generate the py gate from something that was created directly from rust space. I think in the case of TGate and TdgGate we are unconditionally adding them to circuits here: https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/library/standard_gates/equivalence_library.py#L136-L145 when we do import qiskit.

ElePT commented 1 week ago

I removed the PR from the queue because, if I'm not mistaken, it would fail if #12646 got merged. So maybe we can wait until all gates have been added to merge the test.

mtreinish commented 1 week ago

I removed the PR from the queue because, if I'm not mistaken, it would fail if https://github.com/Qiskit/qiskit/pull/12646 got merged. So maybe we can wait until all gates have been added to merge the test.

I think it actually would be fine, because the test only validates if the python side class attribute _standard_gate is set. So for the placeholder gates we've not done that and it'll be a skip. But lets wait for #12646 to merge first and then test my hypothesis, rather than potentially blocking #12646.

coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 9652865752

Details


Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 6 92.62%
crates/qasm2/src/parse.rs 12 97.15%
<!-- Total: 18 -->
Totals Coverage Status
Change from base Build 9650973845: -0.01%
Covered Lines: 63765
Relevant Lines: 71054

💛 - Coveralls
coveralls commented 1 week ago

Pull Request Test Coverage Report for Build 9679858189

Details


Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 7 91.86%
<!-- Total: 7 -->
Totals Coverage Status
Change from base Build 9676629284: 0.009%
Covered Lines: 63792
Relevant Lines: 71062

💛 - Coveralls