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

[WIRES] Allow user to define non-consecutive wires for a device #666

Merged
merged 283 commits into from
Aug 5, 2020

Conversation

mariaschuld
Copy link
Contributor

@mariaschuld mariaschuld commented Jun 4, 2020

Context:

This is the main PR of the wires management refactor. It refactors PennyLane to not assume any more that wires are consecutive integers that can be sorted and compared.

Description of the Change:

The main change is to use the wires argument in device('XXX', wires=..) optionally to pass a sequence of representations or identifiers of wires, i.e. (to name a crazy example).

devlce('default.qubit', wires=['q1', 'q2', 0, 2, 1])

This user wire ordering is converted to a Wires object and stored in the new attribute register. If the old version device('XXX', wires=N) is used, the register is initialised with Wires(range(N)) instead.

The user can now address wires with her labels, i.e. Hadamard(wires=['q2']). The device translates these labels into indices of the wires in the register, i.e. via

wire_indices = self.register.indices(op.wires)

Finally, with this changes we can now let the Wires objects (which do not follow a consecutive logic) pass freely through PennyLane, instead of converting them to lists, as done in the previous PR.

This PR also contains new tests to check that various parts of PennyLane work smoothly for non-consecutive wires.

mariaschuld and others added 30 commits May 14, 2020 16:48
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
@mariaschuld
Copy link
Contributor Author

Regarding @josh146 comment on Tensor observables: I just realised that another problem here is the omission of identities, like in qml.PauliZ(0) @ qml.PauliX(2). Here the Tensor class "knows" that the second position should be qml.PauliZ(1). But what if we have qml.PauliZ('q1') @ qml.PauliX('ancilla')? The only place where PennyLane can interpret this is inside the qnode, because it knows the register...

It is another example of where the wires refactor uncovers how much the independence of the operator module relied on this implicit assumption that we use consecutive wire labels.

@mariaschuld
Copy link
Contributor Author

@josh146, @trbromley , @co9olguy Thanks for your great reviews! No need for action for now, I will ping you when everything is ready (in case you want to have a last look).

As a summary, I addressed the major issues as follows:

  • eigenvalue sorting: I reverted the additions to the wires class to compare Wires objects, and for now compare wires.tolist() objects in the groupby functions of the eigenvalue sorting. Working on solving this issue on a separate branch, and it is actually quite non-trivial.
  • templates integration test being ugly: instead of refactoring that entire test module (which has many implications in how templates are added) I just significantly tidied up the tests and now check for the template's name when making special case exceptions (like setting the wires argument for UCCSD)
  • qchem: Zeyue will kindly work on making qchem wire-order independent asap since it is not clear how to raise a user warning
  • heisenberg_expand function: I very much tidied up here as well, most functions now take only the register and infer the rest. I think we can leave it like this until the CV devices get polished again
  • ActsOn is renamed to WiresEnum

@zeyueN
Copy link
Contributor

zeyueN commented Jul 27, 2020

Hey @mariaschuld ! Just gonna put some of my observations/questions here about the wires, based on what I saw from within the context of qchem. Hopefully not hijacking the comments here and sorry for the long rant.:laughing:

The more I look into it the more it looks like there are multiple things at play here, definitely not as innocent as it initially seemed 😆 . To me, there are two assumptions under the old way of naming wires: 1) consecutive order, and 2) Operator.wires containing wires that match the wires in the now Device.registry. Breaking (1) while keeping (2) unbroken will introduce the side effect of making Operators not independent of devices (ie an Operator has to know about the wire naming/arrangement conversions used by a device when it's defined BEFORE it's stuck into to a QNode, essentially making it weakly attached to certain devices from the outset).

But to keep Operators independent of devices, I feel assumption (2) has to be broken. This means Operator.wires needs to be conceptually different from Device.registry, which kind of makes sense based on my limited knowledge in Pennylane's usage. The former could be for users to name their own wires for whatever reason, and the latter is used for conforming to a devices wire arrangement. And this will require a mapping between the 2 set of wires, likely in qnode when appending operators where it currently raises the "invalid wire" error. Haven't put too much thought into how to do this mapping though (user supplied? naive guessing? some kind of device-specific compiling that takes devices limitations into considerations?) One hacky way, though, of keeping both (2) and device-independence untouched is to enforce users to use the qnode decorator whenever they want to use custom wiring, ie custom wired operators must be device dependent by being in a qnode.

In the context of qchem, if we keep (2) unbroken, the conversion from OpenFermion (who always uses consecutive int cubits) to Pennylane will require the user to supply the wire names of the device he latter is going to run the ansatz on; this could even be before a device is defined. This feels a bit unnatural to me and thus led to the ranting above. Or, I could keep wires as is and experiment with the operator-to-device wire mapping mentioned above.

And the issue about Tensor which we also use extensively in qchem (with the assumption of implicit Identity). My gut feeling is that, if the Tensor's wires doesn't have to conform to a device's registry, then there could be more freedom in telling it the order and all the implicit wires.

Also @agran2018, to keep everyone in the loop.

@josh146
Copy link
Member

josh146 commented Jul 27, 2020

Thanks @zeyueN, some good thoughts. From a very high level, it would be great it we could preserve

  • Operations remaining independent of the device

It would also be quite nice to preserve

  • Assuming consecutive integers by default

as this is a familiar paradigm in other QC libraries (Qiskit, OpenFermion), and allows us to abstract a lot of the more complicated logic. Users running simulations likely will be working with assumed consecutive integers as well, and this would allow them to avoid any additional 'wire label registration'. However, this one is likely highly non-trivial as @mariaschuld was mentioning.

@co9olguy
Copy link
Member

Thanks @zeyueN, your comment raises some good food for thought!

We're thinking of ways for make operators more independently composable outside of QNodes in the future, and this would require a strong separation from the specific devices.

Conceptually, I think we can indeed separate "wire labels" logically from devices.
For instance, being able to do something like
qml.CNOT(wires=["alice", "bob"])
is valuable to users, without ever attaching it to any implementation on a device.

@co9olguy
Copy link
Member

co9olguy commented Jul 27, 2020

  1. Operator.wires containing wires that match the wires in the now Device.registry

Is this the case? ideally, all Operator.wires needs to know about is the user's name for the wires, not how the device handles them, no? (can be assumed consecutive integers on device side, except for Rigetti, where they are hard-wired)

@zeyueN
Copy link
Contributor

zeyueN commented Jul 27, 2020

  1. Operator.wires containing wires that match the wires in the now Device.registry

Is this the case? ideally, all Operator.wires needs to know about is the user's name for the wires, not how the device handles them, no? (can be assumed consecutive integers on device side, except for Rigetti, where they are hard-wired)

This is where I was referring to that matches operator's wires to device's wires:
https://github.com/XanaduAI/pennylane/blob/6a4828d2315555b5e3ecee810e94e5e29368d3e6/pennylane/qnodes/base.py#L370

@co9olguy
Copy link
Member

co9olguy commented Jul 27, 2020

Thanks @zeyueN for finding that one, it definitely looks like something that should be ironed out.
Inspecting the code, it seems operators are weakly coupled to the device, but only if they are created inside a QNode's quantum function (e.g., def circuit()).

🤔 I think what should be checked is that the user's labels for wires in the operations are self-consistent, and can be mapped to corresponding labels on the underlying devices, not necessarily that the user's names for wires match the underlying indices/labels the device prefers (by default, consecutive integers)

mariaschuld and others added 15 commits July 28, 2020 20:02
* rm consec wires assumption in map and Tensor.prune

* treat Wires separately for _flatten
* add wire map functionality to Wires class, and to Device

* Backup

* make syntax more elegant and add tests

* fix all tests

* delete comments

* black

* polish

* polish

* polish2

* fix refactor problem

* small fixes

* one more fix

* add __array__ func to wires

* Update pennylane/_device.py

Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>

* Update pennylane/_device.py

Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>

* Update pennylane/plugins/default_gaussian.py

Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>

* code review comments

* finish code review comments

* remove superfluous line

* [WIRES] Sort observables in Tensor class (#734)

* backuo

* change sorting function

* fix stuff

* remove indexing with wires

* remove more of those instances

* try one more change

* backup

* convert wires to list in map function

Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-ready 👌 PRs which are ready for review by someone from the core team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants