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.

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

Add optional qubit_set property to cirq.Device #2605

Merged
merged 11 commits into from
Dec 17, 2019
Merged

Add optional qubit_set property to cirq.Device #2605

merged 11 commits into from
Dec 17, 2019

Conversation

Strilanc
Copy link
Contributor

@Strilanc Strilanc commented Nov 27, 2019

  • Make 'validate_operation' optional to implement
  • qubit_set falls back to looking for qubit/_qubit fields/methods, otherwise returns None

This makes devices easier to consume (because a common task is to want to view the qubits, knowing that the device is not the unconstrained device) and easier to make (because when prototyping you don't care about validating operations yet).

The shortest interesting device class now looks like this:

class CustomDevice:
    def qubits(self):
        return cirq.LineQubit.range(6)

or this:

class CustomDevice:
    def __init__(self, qubits):
        self.qubits = qubits

- Make 'validate_operation' optional to implement
- qubit_set falls back to looking for qubit/_qubit fields/methods, otherwise returns None
@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Nov 27, 2019
@Strilanc Strilanc requested a review from mpharrigan November 27, 2019 23:53
Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

This seems fine, but I would like to understand a bit more about what we are trying to accomplish with Device. It would be better to have a long-term strategy than have it grow organically.

(Though, having a set of qubits seems pretty standard to a Device, so I don't foresee any issues)

"""Returns a set or frozenset of qubits on the device, if possible.

This method returns None when the set of qubits is unavailable. For
example, `cirq.UnconstrainedDevice` returns `None` because allows any
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is redundant with the Returns statement. I would probably keep one or the other rather than repeating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"""

# Compatibility hack to work with devices that were written before this
# method was defined.
Copy link
Contributor

Choose a reason for hiding this comment

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

As @dstrain115 mentioned, it'd be nice to have a plan for where we are going with Device so that we can converge on an interface in the future and get rid of this hack. Maybe create an issue to track this and reference the issue number here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged.


class PrivateQubitMethodDevice(cirq.Device):

def qubits(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be _qubits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mpharrigan
Copy link
Collaborator

mpharrigan commented Dec 2, 2019

echoing others' comments about long-term-plan. "Qubits" are inexorably linked to gate sets.

Is set the right data structure here?

  1. introduces asymmetry between the constructor and the property. You give a list but duplicates and order is "erased"
  2. Confusion around the order of the returned thing and/or you can't index in

I might want to write:

qubits = Sycamore.qubits
cirq.Circuit(cirq.CNOT(qubits[0], qubits[1]))

but that's not possible, so I might write

qubits = list(Sycamore.qubit_set())
....

and then wonder why my program is different after python changes set ordering / we change how qubits are fed into the device constructor / who knows what

@Strilanc
Copy link
Contributor Author

Strilanc commented Dec 2, 2019

and then wonder why my program is different after python changes set ordering / we change how qubits are fed into the device constructor / who knows what

Sets preserve order in python 3, so you shouldn't see the order change like that.

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 2, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 2, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 2, 2019

Automerge cancelled: A status check is failing.

@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 Dec 2, 2019
@Strilanc
Copy link
Contributor Author

I have discovered that sets do not preserve order in python 3, only dictionaries.

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 17, 2019

Automerge cancelled: There are merge conflicts.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 17, 2019

Automerge cancelled: A status check is failing.

@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 Dec 17, 2019
@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019
@CirqBot CirqBot merged commit 1309e99 into master Dec 17, 2019
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019
@CirqBot CirqBot deleted the device_qubits branch December 17, 2019 07:51
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019
@mpharrigan
Copy link
Collaborator

I have discovered that sets do not preserve order in python 3, only dictionaries.

See my concerns above. I think it will be more helpful to have ordered qubits in a device

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants