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

Small refactoring of unitary synthesis tests #13582

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

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Dec 19, 2024

Summary

This is a small refactoring PR that proposes splitting the UnitarySynthesis test class into two separate classes, one for tests that follow the basis_gates path, which is still in Python, and one for the target path, which is now in Rust. With the current interleaving of target and basis gates tests it's easy to oversee what is covered where, and I believe that as a result we have an improvable coverage of both the Python and Rust UnitarySynthesis code. This PR doesn't fix that but sets the stage for reviewing and adding new tests in an organized way as we work on PRs like #13568.

Details and comments

As I was moving the tests I took the time to squash a few of them into one, as they often just varied in one parameter that could be handled by ddt.

@ElePT ElePT requested a review from a team as a code owner December 19, 2024 12:13
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@@ -673,38 +569,6 @@ def test_coupling_map_transpile_with_backendv2(self, opt_level, bidirectional):
(0, 1), (circ_01_index[instr.qubits[0]], circ_01_index[instr.qubits[1]])
)

@data(1, 2, 3)
def test_coupling_map_unequal_durations(self, opt):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this test because we originally had it as the backend V1 version of test_coupling_unequal_duration_with_backendv2, it was then migrated to backend v2 and became essentially a duplicate of the other one.

@coveralls
Copy link

coveralls commented Dec 19, 2024

Pull Request Test Coverage Report for Build 12687926263

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.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 370 unchanged lines in 13 files lost coverage.
  • Overall coverage decreased (-0.02%) to 88.942%

Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/optimization/collect_multiqubit_blocks.py 2 98.28%
qiskit/circuit/library/standard_gates/rz.py 3 95.71%
qiskit/circuit/library/standard_gates/rx.py 3 95.95%
qiskit/circuit/library/standard_gates/ry.py 3 95.83%
crates/qasm2/src/lex.rs 4 92.23%
qiskit/primitives/containers/observables_array.py 5 95.0%
crates/accelerate/src/commutation_checker.rs 16 97.41%
qiskit/quantum_info/operators/symplectic/pauli_list.py 16 82.93%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py 20 94.58%
crates/accelerate/src/unitary_synthesis.rs 23 93.18%
Totals Coverage Status
Change from base Build 12395850432: -0.02%
Covered Lines: 79444
Relevant Lines: 89321

💛 - Coveralls

@ShellyGarion ShellyGarion self-assigned this Jan 9, 2025
@ShellyGarion ShellyGarion added the Changelog: None Do not include in changelog label Jan 12, 2025
Copy link
Member

@ShellyGarion ShellyGarion left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ElePT for the tests refactoring, it's very helpful!

It's not part of this PR, but I would just like to point out that it seems that the XXDecomposer lines inside the unitary_synthesis transpiler pass are not being covered by any of the tests, see the coverage report:
https://coveralls.io/builds/71414685/source?filename=qiskit%2Ftranspiler%2Fpasses%2Fsynthesis%2Funitary_synthesis.py

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants