-
Notifications
You must be signed in to change notification settings - Fork 127
Unitary Layer RB #1561
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
Unitary Layer RB #1561
Conversation
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.
Looks pretty good!
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
…ity_unitary.py Co-authored-by: Will Shanks <[email protected]>
releasenotes/notes/add_layer_fidelity_unitary-bad3cc81c0c5f959.yaml
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
qiskit_experiments/library/randomized_benchmarking/layer_fidelity_unitary.py
Outdated
Show resolved
Hide resolved
@wshanks ![]() |
Looks like that's only for python 3.10 and we still run 3.9 tests |
Ah, okay, using |
# Add the fold rzz angle pass | ||
# pylint: disable=no-member | ||
if "rzz" in basis_gates: | ||
pass_manager_2q.translation.append([FoldRzzAngle()]) |
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.
Okay, one last suggestion for avoiding doing this -- if we pass a backend
to generate_preset_pass_manager
any custom transpiler passes should get picked up through the plugin system. So we could pass self.backend
here. I think a reason we do not do that now is that generate_preset_pass_manager
warns not to pass both a backend and basis_gates/coupling_map. I believe when you pass basis_gates
and coupling_map
to the transpiler it just generates a target internally any way. So we could instead just create the target and pass that, which does not generate a warning, like this:
target = Target.from_configuration(basis_gates=basis_gates, coupling_map=CouplingMap(((0, 1),)))
pass_manager = generate_preset_pass_manager(backend=self.backend, target=target)
I don't think we need separate 1q and 2q pass managers in this case.
@mtreinish Could you confirm if this approach is idiomatic Qiskit?
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 guess I was also thinking this might weigh the transpiler down more? but I didn't test it
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.
Passing the backend and target? I don't think so because the transpiler will still build a target internally. If the cost of building the transpiler were a concern, we could build it once at a higher level instead of building it here for each circuit. It is probably not a bottle neck though.
Looking at the generate_preset_pass_manager
code, it seems to do what we want in terms of taking the gates and coupling map from the target and the other things like dt and durations from the backend. I think we don't care about those things any way but I am not sure if it is better in general to copy them from the backend into the new target or not.
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.
Passing both the backend and the target will work. If you do that the target
argument takes precedence over the backend.target
which will be ignored. But the default pipeline method from the backend should still get picked up. If you're looking to run the custom passes from a backend that would work. I don't think we have test coverage of that particular usage but quickly scanning the code it matches the documented behavior.
As an alternative if you wanted to use a bit more explicit syntax you could do:
pass_manager = generate_preset_pass_manager(
target=target,
translation_method=backend.get_translation_stage_plugin(),
scheduling_method=backend.get_scheduling_stage_plugin(),
)
but either is valid.
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.
So I tried one pass manager for both 1Q and 2Q but then in this line
qc_tmp = pass_manager.run(qc_tmp)
circ._append(qc_tmp.to_instruction(), (circ.qubits[q],), ())
it was actually turning that into a 2Q instruction and this was causing issues... so I went back to 1Q and 2Q pass manager (as pushed)
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 used the version where we pass the target and backend. @mtreinish 's example would require me to check if those calls exist in the backend first and so seemed a bit more messy
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.
The code looks good. Just one question about the release notes.
This PR adds a new class to do layer style RB with an arbitrary set of 2Q unitary circuits for the entangling layer. Example usage which will perform RB with 10 different RZZ angles.