-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
base: main
Are you sure you want to change the base?
Conversation
One or more of the following people are relevant to this code:
|
@@ -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): |
There was a problem hiding this comment.
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.
Pull Request Test Coverage Report for Build 12687926263Warning: 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
💛 - Coveralls |
There was a problem hiding this 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
Summary
This is a small refactoring PR that proposes splitting the
UnitarySynthesis
test class into two separate classes, one for tests that follow thebasis_gates
path, which is still in Python, and one for thetarget
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 RustUnitarySynthesis
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.