-
Notifications
You must be signed in to change notification settings - Fork 2
JCZ transpiler revision #1
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
base: main
Are you sure you want to change the base?
Conversation
mgarnier59
left a comment
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 good, thanks for the work!
Some of my comments span the original version that Thierry has done.
Two quick comments/questions:
- the well-formedness check on patterns directly depend on the well-formedness of the circuit. Are we doing this here because it's not yet implemented for graphix
Circuit? I think separating concerns is good. - I'm not clear on the
JCZinstructiontype. It seems to me that we would need only sequences of circuit instructions and sequences of JCZ so I don't understand why we would want to mix those.
Also I don't think we took into account the case of a CZ gate in the circuit (but I might be wrong on that point)
| ------ | ||
| IllformedPatternError: if the pattern is ill-formed. | ||
| InternalInstructionError: if the circuit contains internal _XC or _ZC instructions. | ||
| IllformedPatternError: if the circuit has underdefined instructions. |
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.
It's strange that an ill-formed circuit raises a pattern error.
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.
Yes agreed. Fixed.
|
|
||
|
|
||
| def instruction_to_jcz(instr: JCZInstruction) -> Iterable[J | CZ]: | ||
| def instruction_to_jcz(instr: JCZInstruction) -> Sequence[J | CZ]: |
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.
On line 263-264 (unchanged): I agree it's weird to have an explicit identity in a circuit. Formally we could always compile the identity into two Hadamard (hence J(0) o J(0)). Maybe comment that we explicitly remove identities? I don't think it's very important though.
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.
Yes it's odd, but I think this is the easiest way to deal with it and keep consistent outputs from the functions. Happy to leave this as is.
|
|
||
|
|
||
| def instruction_to_jcz(instr: JCZInstruction) -> Iterable[J | CZ]: | ||
| def instruction_to_jcz(instr: JCZInstruction) -> Sequence[J | CZ]: |
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.
Is this function not suppose to take circuit instructions to JCZ? Hence the type annotation has to be modified. From JCZInstruction to JCZInstruction | Instruction.
What I don't understand is why we want to allow feeding a JCZInstructionto this function since it's just the identity on these objects. Circuits shouldn't contain such instructions.
As a side remark, the case where the circuit explicitely contains a CZ gate is not taken into account (I believe)
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.
JCZInstruction is already defined as the union Instruction | J.
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.
Yes, so to go from the bottom:
- CZ gates are not in Graphix (yet) so shouldn't be a problem. I will add them in if/when my PR is merged.
- JCZInstruction is defined as J | CZ | Instruction, so yes it will be the identity on J and CZ inputs. It is general because the function is recursive (might input CNOT, decomposed to H CZ H, which goes through the
instruction_list_to_jczand each is then sent toinstruction_to_jcz.) So yes the initial inputs should just be Instruction type, but since the function is used for intermediate steps the typing needs to be the union.
| ] | ||
|
|
||
|
|
||
| def transpile_jcz(circuit: Circuit) -> TranspileResult: |
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.
Comment for line 374: why do we expect the circuit to contain _XCor _ZC instructions?
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.
We don't, but unfortunately the instructions _XC and _ZC exist in graphix (they are used internally by the original transpiler, so I hope they will disappear eventually).
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.
maybe remove the . vscode that is local?
| transpiled.pattern.perform_pauli_measurements() | ||
| transpiled.pattern.minimize_space() | ||
|
|
||
| def simulate_and_measure() -> int: |
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.
In this test, I would add a comment saying what we are doing: since it's a circuit transpiled in JCZ, the pattern has causal flow so all measurement outcomes have probability 1/2 to occur (up to statistical fluctuations).
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.
Done
| def decompose_ccx( | ||
| instr: instruction.CCX, | ||
| ) -> list[instruction.H | instruction.CNOT | instruction.RZ]: | ||
| ) -> Sequence[instruction.H | instruction.CNOT | instruction.RZ]: |
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.
We usually follow the convention of being as precise as possible in the returned datatype, so we prefer the type list over Sequence if the return value is a list. This practice is Python-specific. In a statically typed language without downcasting, it would make sense to hide the concrete data type in the interface for better encapsulation. In Python, since callers can freely treat the result as a list if it is one, the most helpful type annotation is the concrete type.
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 will try to commit with this changed back, but I get typing issues.
Something to do with Sequences being covariant, and lists not being.
May be able to keep as lists but broaden their typing to JCZInstruction.
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.
Could you share the type error messages?
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.
For example, where decompose_y is called in the instruction_to_jcz, I get the error ""Argument 1 to instruction_list_to_jcz" has incompatible type "list[X | Z]"; expected "list[CCX | RZZ | CNOT | SWAP | H | <10 more items>]""
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.
It also adds ""List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
Consider using "Sequence" instead, which is covariant"
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 propose a fix in this commit. There were two issues:
instruction_list_to_jcztook alisteven though it only needs anIterable. If a functionfis defined with a parameterl: list[A], that parameter is mutable, so it would be unsafe to callfwith an argument of typelist[B]withBa subtype ofA:fcould append anyAtol, including values that are not of typeB. That's whylistis invariant.Iterableprovides read-only access and is covariant; sincelist[B]is anIterable[B], it is also anIterable[A]whenBis a subtype ofA.- The return type of
instruction_to_jczwaslist[J | CZ]but some branches returnlist[J]. I changed the return type toSequence[J | CZ]which is a common supertype of bothlist[J | CZ]andlist[J]. Alternatively,list[J | CZ] | list[J]would also work.
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.
Thanks Thierry. Makes sense to me. Based on your logic above, should we not use Sequence rather than Iterable for instruction_list_to_jcz?
(Update: Ignore this question, I slept on it and it makes complete sense you would maximise generality on inputs and specificity on outputs.)
|
|
||
|
|
||
| def instruction_to_jcz(instr: JCZInstruction) -> Iterable[J | CZ]: | ||
| def instruction_to_jcz(instr: JCZInstruction) -> Sequence[J | CZ]: |
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.
JCZInstruction is already defined as the union Instruction | J.
| ] | ||
|
|
||
|
|
||
| def transpile_jcz(circuit: Circuit) -> TranspileResult: |
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.
We don't, but unfortunately the instructions _XC and _ZC exist in graphix (they are used internally by the original transpiler, so I hope they will disappear eventually).
…ation on larger patterns produced through Open Graphs.
Update the code for the latest Graphix master API. Disable Pauli preprocessing in `test_circuit_simulation_og` because it can break the flow (graphix#363), and `minimize_space` does not work well when no flow is available (graphix#157).
|
@thierry-martinez @mgarnier59 let's review again first thing next week. I can update with the changes to Graphix main once the |
This commit introduces minor changes required to support interfacing with a brickwork transpiler (future "PR").
Changes include:
list/IterabletoSequencetypes to match usage across JCZ and brickworkj_commandswhich is used across both modules