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.

Sign up for GitHub

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

Jump to bottom

Migrate ionq_device compilation to circuit transformer #4901

Closed
Cynocracy opened this issue Jan 27, 2022 · 13 comments
Closed

Migrate ionq_device compilation to circuit transformer #4901

Cynocracy opened this issue Jan 27, 2022 · 13 comments
Assignees
@Cynocracy @tanujkhattar
Labels
area/transformers kind/task A task that's part of a larger effort triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@Cynocracy
Copy link
Contributor

Summarize the task
Spin-off from #4824 to get ionq device prepped for deprecation of decompose_operation API in devices.

Acceptance criteria - when is the task considered done?
When ionq_device is no longer blocking deprecation of decompose_operation
@Cynocracy Cynocracy added the kind/task A task that's part of a larger effort label Jan 27, 2022
@Cynocracy Cynocracy mentioned this issue Jan 27, 2022
Merged
@MichaelBroughton MichaelBroughton added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on area/transformers labels Jan 31, 2022
@tanujkhattar tanujkhattar mentioned this issue Feb 2, 2022
Merged
@tanujkhattar
Copy link
Collaborator

@Cynocracy Cirq optimizers have been migrated to new transformer framework. Can you now do the migration for ionq device? You should create a new TwoQubitCompilationTargetGateset for ionq put the decomposition logic there. See other similar implementations of gatesets, like CZTargetGateset in cirq core.

Please let me know if you have any questions.

@tanujkhattar
Copy link
Collaborator

#5028 This issue tracks the migration of optimizers in vendor packages

@Cynocracy
Copy link
Contributor Author

Sure thing, happy to take this one up

@Cynocracy
Copy link
Contributor Author

I was looking into this, and found #4925 recently that seems to have made changes to how cirq-ionq exposes these, but it doesn't support nqubits > 2 which is strange.

Anyway, it's making my changes a bit more involved than I intiially thought, so just calling it out.

Am shooting to continue to support that high level interface but to use a [TwoQubitCompilationTargetGateset "behind the scenes" (while also dropping IonqAPIDevice)

@Cynocracy
Copy link
Contributor Author

Hmm, maybe I'm just not understanding. The diff looks like it tried to preserve behavior.

Going to continue to try to wrap my head around things :) hopefully a PR emerges soon

@Cynocracy Cynocracy mentioned this issue Mar 22, 2022
Closed
@Cynocracy
Copy link
Contributor Author

Have my changes staged, but am confused by the sudden appearance of cirq.PhasedXZGate in the test output :( I've tried using the base class and TwoQubit[...]

@Cynocracy
Copy link
Contributor Author

The full failure is

>           assert VALID_DECOMPOSED_GATES.validate(decomposed_circuit)
E           assert False
E            +  where False = <bound method Gateset.validate of cirq.Gateset(cirq.ops.common_gates.XPowGate, cirq.ops.common_gates.ZPowGate, cirq.CNOT, unroll_circuit_op = True,accept_global_phase_op = True)>(cirq.Circuit([\n    cirq.Moment(\n        cirq.PhasedXZGate(axis_phase_exponent=0.11059847571375991, x_exponent=0.3757376732242559, z_exponent=-0.9918267391627413).on(cirq.LineQubit(0)),\n    ),\n]))
E            +    where <bound method Gateset.validate of cirq.Gateset(cirq.ops.common_gates.XPowGate, cirq.ops.common_gates.ZPowGate, cirq.CNOT, unroll_circuit_op = True,accept_global_phase_op = True)> = cirq.Gateset(cirq.ops.common_gates.XPowGate, cirq.ops.common_gates.ZPowGate, cirq.CNOT, unroll_circuit_op = True,accept_global_phase_op = True).validate

And I'm puzzled as how I've changed this behavior at all.

Any ideas?

@Cynocracy
Copy link
Contributor Author

mm, I'm wondering if it's the yield statements and me not understanding where/why things are generators.

@MichaelBroughton MichaelBroughton mentioned this issue Mar 22, 2022
Closed
@Cynocracy
Copy link
Contributor Author

I am stuck again, still ending up with a cirq.PhasedXZGate in the compiled circuit, which should be broken up. Pushed my latest yield-less changes

@tanujkhattar
Copy link
Collaborator

@Cynocracy You also need to override the postprocess_transformers property of your compilation target gateset and remove merge_single_qubit_moments_to_phxz transformer phase.

See

Cirq/cirq-core/cirq/transformers/target_gatesets/compilation_target_gateset.py

Line 114 in 45624ff

merge_single_qubit_gates.merge_single_qubit_moments_to_phxz,

@dstrain115
Copy link
Collaborator

See also #5129

@dstrain115
Copy link
Collaborator

@Cynocracy What is the status of this PR? Do you still need help with this? It looks like Tanuj had a suggestion in how to fix this. Do you think we could either wrap this up, or expose the branch for someone else to finish?

@tanujkhattar tanujkhattar mentioned this issue Jun 10, 2022
Merged
CirqBot pushed a commit that referenced this issue Jun 13, 2022
@tanujkhattar
Add IonQTargetGateset and use it for circuit compilations. (#5479)
3360198 
Fixes #5129 #4901 #4153
Replaces #5127

* Adds support for compiling > 2q operations to ionq's target gateset
* Does not do the "merge 2q operations to a connected component" optimization, to preserve existing behavior. We can add another `preprocess_transformer` if we want to do the optimization. 
* Deprecates `cirq_ionq.decompose_to_device`  in favour of using `cirq.optimize_for_target_gateset(circuit, gateset=cirq_ionq.IonQTargetGateset)` for compiling circuits. 

cc @Cynocracy @dabacon
@tanujkhattar
Copy link
Collaborator

This is fixed by #5479

rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@tanujkhattar @rht
Add IonQTargetGateset and use it for circuit compilations. (quantumli…
cefd856 
…b#5479)

Fixes quantumlib#5129 quantumlib#4901 quantumlib#4153
Replaces quantumlib#5127

* Adds support for compiling > 2q operations to ionq's target gateset
* Does not do the "merge 2q operations to a connected component" optimization, to preserve existing behavior. We can add another `preprocess_transformer` if we want to do the optimization. 
* Deprecates `cirq_ionq.decompose_to_device`  in favour of using `cirq.optimize_for_target_gateset(circuit, gateset=cirq_ionq.IonQTargetGateset)` for compiling circuits. 

cc @Cynocracy @dabacon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Cynocracy Cynocracy

@tanujkhattar tanujkhattar

Labels
area/transformers kind/task A task that's part of a larger effort triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dabacon @Cynocracy @MichaelBroughton @tanujkhattar @dstrain115