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

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

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

mtreinish
Copy link
Member

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

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.
@mtreinish mtreinish added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Jun 20, 2024
@mtreinish mtreinish requested a review from a team as a code owner June 20, 2024 21:23
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Jun 20, 2024

Pull Request Test Coverage Report for Build 9604586227

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 10 unchanged lines in 2 files lost coverage.
  • Overall coverage remained the same at 89.74%

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

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

I think that the test is not behaving as intended, shouldn't it fail without fixing the wrong import paths we currently have for the T gates? (I did check it out locally and there it fails as expected). Maybe something related to the test config in CI?

@mtreinish
Copy link
Member Author

mtreinish commented Jun 21, 2024

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.

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Ok, so we might not be able to catch the fallback path on CI but the test is still good to have. I guess the best we can do in this case is keep this in mind and run the test locally when relevant.

@ElePT ElePT added this pull request to the merge queue Jun 24, 2024
@ElePT ElePT removed this pull request from the merge queue due to a manual request Jun 24, 2024
@ElePT
Copy link
Contributor

ElePT commented Jun 24, 2024

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
Copy link
Member Author

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.

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
Copy link

coveralls commented Jun 24, 2024

Pull Request Test Coverage Report for Build 9652865752

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 18 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.742%

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

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 26, 2024

Pull Request Test Coverage Report for Build 9679858189

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 7 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.009%) to 89.769%

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

💛 - Coveralls

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Looks to be working!

@ElePT ElePT enabled auto-merge June 26, 2024 13:09
@ElePT ElePT added this pull request to the merge queue Jun 26, 2024
Merged via the queue into Qiskit:main with commit 39b2c90 Jun 26, 2024
15 checks passed
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 mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants