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

Remove deprecated classes ConvertToIonGates and IonDevice #5696

Merged
merged 14 commits into from
Jul 13, 2022
Merged

Remove deprecated classes ConvertToIonGates and IonDevice #5696

merged 14 commits into from
Jul 13, 2022

Conversation

pavoljuhas
Copy link
Collaborator

@pavoljuhas pavoljuhas commented Jul 8, 2022 •
edited

Blend _IonDeviceImpl implementation superclass into AQTDevice.
Also copy ion_device_test tests into aqt_device_test.

Co-authored-by: Tanuj Khattar tanujkhattar@google.com

@pavoljuhas pavoljuhas requested review from ma5x, pschindler, a team, vtomole and cduck as code owners July 8, 2022 20:37
@pavoljuhas pavoljuhas requested a review from maffoo July 8, 2022 20:37
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Jul 8, 2022
@pavoljuhas pavoljuhas mentioned this pull request Jul 8, 2022
Closed
@pavoljuhas
Temporarily ignore coverage of the moved lines
9330dff 
Add TODO issue to fix the coverage later.
dstrain115
dstrain115 reviewed Jul 8, 2022
View reviewed changes
cirq-aqt/cirq_aqt/aqt_device.py Outdated
class AQTTargetGateset(cirq.ion.ion_device._IonTargetGateset):
pass

class AQTTargetGateset(transformers.TwoQubitCompilationTargetGateset):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a docstring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a docstring?

@tanujkhattar - could you please help with this? (I think you are the original author of that class)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a suggestion, PTAL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a suggestion, PTAL.

committed, thank you!

cirq-aqt/cirq_aqt/aqt_device.py
def decompose_circuit(self, circuit: circuits.Circuit) -> circuits.Circuit:
return transformers.optimize_for_target_gateset(circuit, gateset=self.gateset)

def duration_of(self, operation):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we move durations into metadata?
Maybe @MichaelBroughton can advise?

I think main device class is for validation and metadata is for "exploration" (i.e. understanding the topology of the device).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be moved to metadata yes.

cirq-aqt/cirq_aqt/aqt_device.py Outdated
q = devices.LineQubit(position)
return q if q in self.qubits else None

def neighbors_of(self, qubit: devices.LineQubit) -> Iterable[devices.LineQubit]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely move to metadata as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be moved as well.

@MichaelBroughton
Copy link
Collaborator

MichaelBroughton commented Jul 11, 2022 •
edited

The duration_of and neighbors_of functions can be moved into MetaData, but perhaps that is best done in a seperate PR, I think this is fine to merge as is (the PR is pretty big already).

@pavoljuhas pavoljuhas mentioned this pull request Jul 11, 2022
Closed
@pavoljuhas
Copy link
Collaborator Author

The duration_of and neighbors_of functions can be moved into MetaData, but perhaps that is best done in a seperate PR, I think this is fine to merge as is (the PR is pretty big already).

Added reminder issue #5726.

tanujkhattar
tanujkhattar reviewed Jul 12, 2022
View reviewed changes
cirq-aqt/cirq_aqt/aqt_device.py Outdated Show resolved Hide resolved
cirq-aqt/cirq_aqt/aqt_device.py Outdated Show resolved Hide resolved
@pavoljuhas
Copy link
Collaborator Author

@dstrain115 , @MichaelBroughton , @tanujkhattar - I feel this should be good to go, can one of you PTAL?

@pavoljuhas pavoljuhas added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 13, 2022
tanujkhattar
tanujkhattar requested changes Jul 13, 2022
View reviewed changes
cirq-aqt/cirq_aqt/aqt_device.py Outdated Show resolved Hide resolved
@tanujkhattar tanujkhattar removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 13, 2022
@pavoljuhas
Shorten cirq.module.Something to cirq.Something
d385c9a 
AQTDevice is in a separate cirq_google package which can refer to
cirq objects directly without worries of circular imports.
@pavoljuhas pavoljuhas added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 13, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 13, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented Jul 13, 2022

Automerge cancelled: A required status check is not present.

Missing statuses: ['Misc check', 'Notebook formatting', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Type check', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']

@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 13, 2022
@pavoljuhas pavoljuhas added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 13, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 13, 2022
@CirqBot CirqBot merged commit 9e61e0e into quantumlib:master Jul 13, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jul 13, 2022
@pavoljuhas pavoljuhas deleted the drop-cirq.ion-deprecations branch July 13, 2022 22:57
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
@pavoljuhas @tanujkhattar @rht
Remove deprecated classes ConvertToIonGates and IonDevice (quantumlib…
66f0827 
…#5696)

Blend `_IonDeviceImpl` implementation superclass into AQTDevice.
Also copy `ion_device_test` tests into `aqt_device_test`.

Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanujkhattar tanujkhattar tanujkhattar approved these changes

@dstrain115 dstrain115 dstrain115 approved these changes

@ma5x ma5x Awaiting requested review from ma5x

@pschindler pschindler Awaiting requested review from pschindler

@vtomole vtomole Awaiting requested review from vtomole

@cduck cduck Awaiting requested review from cduck

@maffoo maffoo Awaiting requested review from maffoo maffoo was automatically assigned from quantumlib/cirq-maintainers

@MichaelBroughton MichaelBroughton Awaiting requested review from MichaelBroughton

Assignees
No one assigned
Labels
size: L 250< lines changed <1000
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pavoljuhas @MichaelBroughton @CirqBot @tanujkhattar @dstrain115