Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 21581584907Details
💛 - Coveralls |
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks Julien, the new method is considerably nicer from the API perspective. The PR looks great (modulo some docs errors).
I have a few higher-level questions.
First, we will likely want similar functions but with different basis sets, e.g. Clifford+RZ+T, or just Clifford. Do you envision that we will add similar methods build_clifford_t_rz, etc.?
Second, thinking of our Clifford+T transpiler pipeline work, suppose that we already have a Clifford+T target that we want to compile to, and suppose that we will want to construct a new intermediate target that also contains RZ gates and has CX gates going in both directions. Would it make sense to add a method to Target to produce such an "abstraction" from an existing target?
| basis_gates = get_clifford_gate_names() + ["t", "tdg"] | ||
| return Target.from_configuration(basis_gates, num_qubits, coupling_map) |
There was a problem hiding this comment.
Do we want to expose other options of from_configuration to build_target?
| @@ -0,0 +1,22 @@ | |||
| --- | |||
| features_transpiler: | |||
| - Added :meth:`.Target.build_clifford_t` to conveniently build a | |||
There was a problem hiding this comment.
I didn't know that we have this syntax: previously I always wrote release notes as in the suggestion below (this is only for illustration, so do not merge!)
| - Added :meth:`.Target.build_clifford_t` to conveniently build a | |
| - | | |
| Added :meth:`.Target.build_clifford_t` to conveniently build a |
jakelishman
left a comment
There was a problem hiding this comment.
I don't deny this is a shortcut for the current API, but it's not clear to me that leaning all in on the existing Target to represent Clifford+T basis sets longer term is a good idea.
Yeah, the existing Target can represent Clifford+T basis sets, but we always have to re-infer that fact back off it later, which we do in a variety of messy and non-identical ways. Things like gate duration and individualised error rates for particular instructions don't seem like an especially good or meaningful representation, and this Target doesn't allow arbitrary or genericised "clifford" instructions, so it's more like "T and Tdg, plus Cliffords but decomposed to the particular set of built-ins Qiskit actually has", rather than "compile to a structured Clifford and extra 'magic'".
Personally, I don't think this is a clean API addition. It feels more like promoting an existing hack up to being blessed as "public API", especially because the internal code is so simple and so opinionated.
|
Thanks for bringing up these very valid points, I agree we eventually want a more formal separation here -- this PR was less ambitious here and has the main goal of having a convenient way of triggering the Clifford+T pipeline 😄 Maybe there's a way we can make an incremental step towards that here
Yeah it would be better if we could set a Target type or "class" in a generic way (not just near-term vs. Clifford+T). The optimization metric also goes into this direction.
Error rates and duration would make likely be important, not only for sub-groups (Cliffords, Paulis, T), but also depending on the locations (Cliffords on specific indices might be more expensive than others).
Agreed that we could add support for e.g. arbitrary |
|
Thanks Jake. We should definitely at some point improve the code based on the points you raised. This issue is related to how we present the nisqy Clifford+T transpiler pipeline to users. When Julien presented the ongoing FTQC work in one of the meetings, several people have explicitly asked for a more convenient way to invoke the pipeline, without having to be aware of the more obscure |
|
Seconding Jake’s general feedback about the API aspect, I also think it’s better to keep the |
Summary
Added
Target.build_clifford_tto conveniently build aTarget for Clifford+T transpilation. This target includes
all standard Clifford gates (see
get_clifford_gate_names)T and Tdg.
Closes #15632.
Details and comments
This internally uses
Target.from_configuration, and maybe should be calledfrom_clifford_tfor consistency.The initial plan was to build the Target Rust-first and expose the same functionality to C, but the C-exposed Target and the Python one have lots of differences which makes using a single Rust-source complicated. I'll add a second PR to expose this to C too.
For example: