Created ControlValues for controlled gates/operations, fix for #4512 #4714

NoureldinYosri · 2021-11-30T19:47:35Z

created control_values.py which contains the ControlValues class.
FreeVars and ConstrainedVars classes are provided for ease of use.
while the basic idea of ControlValues integrating it inside the code base was challening
the old way of using control_values assumed it's a tuple of tuples of ints and was used as thus (comparasion, hashing, slicing, fomatting, conditioning, and loops), the ControlValues class had to provide these functionalities
the trickiest part to get right was the support for formatting!
fixes Controlled gate construction cannot construct some cases #4512

…mlib#4512 quantumlib#4714 - created control_values.py which contains the ControlValues class. - FreeVars and ConstrainedVars classes are provided for ease of use. - while the basic idea of ControlValues integrating it inside the code base was challening - the old way of using control_values assumed it's a tuple of tuples of ints and was used as thus (comparasion, hashing, slicing, fomatting, conditioning, and loops), the ControlValues class had to provide these functionalities - the trickiest part to get right was the support for formatting! - I'll create a follow up PR for unit tests for control_values.py

tanujkhattar · 2021-12-02T08:12:39Z

Thanks @NoureldinYosri for opening up the PR. Unit tests need to be added for every new code / modification or else the coverage check won't pass and we can't approve any PRs with failing tests. Please add the unit tests (and fix other failing tests) and then we can do a review for the PR.

You can look at https://github.com/quantumlib/Cirq/blob/master/docs/dev/development.md#continuous-integration-and-local-testing to learn more about how to run the integration tests locally.

NoureldinYosri · 2021-12-11T15:31:25Z

@tanujkhattar can you check this PR?, all checks pass now :)

dabacon

Some initial comments.

cirq-core/cirq/ops/control_values.py

cirq-core/cirq/ops/controlled_gate.py

cirq-core/cirq/ops/control_values.py

cirq-core/cirq/ops/control_values_test.py

cirq-core/cirq/ops/control_values.py

cirq-core/cirq/ops/control_values_test.py

cirq-core/cirq/ops/control_values.py

cirq-core/cirq/ops/controlled_gate_test.py

…mlib#4512 created control_values.py which contains the ControlValues class. FreeVars and ConstrainedVars classes are provided for ease of use. while the basic idea of ControlValues integrating it inside the code base was challening the old way of using control_values assumed it's a tuple of tuples of ints and was used as thus (comparasion, hashing, slicing, fomatting, conditioning, and loops), the ControlValues class had to provide these functionalities the trickiest part to get right was the support for formatting!

NoureldinYosri · 2022-01-17T08:52:59Z

@daxfohl @dabacon this PR has been waiting for sometime, can we get it reviewed :)

daxfohl

Some quick comments from my phone.

daxfohl · 2022-01-17T18:45:48Z

cirq-core/cirq/ops/controlled_gate.py

-                    'Control values <{!r}> outside of range for control qubit '
-                    'number <{!r}>.'.format(val, i)
+        if not isinstance(control_values, cv.ControlValues):
+            self.control_values = cv.ControlValues(


This needs to be marked as a breaking change since it changes the data type of a public field. @tanujkhattar

cirq-core/cirq/ops/controlled_gate.py

daxfohl · 2022-01-17T18:51:46Z

cirq-core/cirq/ops/control_values.py

+    return tuple(tuple(product) for product in vals)
+
+
+class ControlValues:


All public functions need docstrings. Also this class needs to be available in the Cirq root namespace.

what is the root namespace?

The root __init__.py file, so users can do cirq.ControlValues. (Once you do this you'll find it triggers some additional tests that check for serializability etc. These tasks need to be completed too, or else the control won't be able to run on real machines).

cirq-core/cirq/ops/control_values.py

daxfohl · 2022-01-17T19:12:58Z

Can you add "fixes #4512" to the description so they are linked?

daxfohl

One more thing

cirq-core/cirq/ops/control_values.py

…mlib#4512 created control_values.py which contains the ControlValues class. FreeVars and ConstrainedVars classes are provided for ease of use. while the basic idea of ControlValues integrating it inside the code base was challening the old way of using control_values assumed it's a tuple of tuples of ints and was used as thus (comparasion, hashing, slicing, fomatting, conditioning, and loops), the ControlValues class had to provide these functionalities the trickiest part to get right was the support for formatting!

NoureldinYosri · 2022-01-28T16:59:30Z

@daxfohl I addressed most of the comments, I have a couple of questions which I put in the comments.

cirq-core/cirq/ops/control_values.py

daxfohl · 2022-01-29T01:21:55Z

@NoureldinYosri consider using @dataclass(frozen=True) on this to help ensure immutability. We use that elsewhere in cirq, you can just search for examples. It provides nice default implementation of equality, hash, etc as well. Just an option--if it gets in the way then don't worry about it.

…plementations, SimpleControlValues which represents the old format of control_values, ConstrainedControlValues which can be used to represent more complex expressions (e.g. xor), this allows us to later add concrete implementations that support other formats (e.g. sympy expressions), I also added a builder class to facilitate the creation of ControlValues.

NoureldinYosri · 2022-02-23T19:14:29Z

@daxfohl @dabacon @tanujkhattar ptal

daxfohl · 2022-02-24T07:51:51Z

The changes resolve my concerns about mutability. Still need to add these classes to the root namespace in cirq-core/cirq/__init__.py. Also the classes' top level docstrings need some more detailed explanation and examples suitable for end users. Beyond that, I think I'll pass this along to the maintainers at this point.

MichaelBroughton · 2022-03-03T18:50:55Z

There are some concerns here re: qsim compatability @95-martin-orion , this has implications for TFQ as well and our goals for decompose always returning X,Y,Z,C*Z. Have you looked into this Orion ?

95-martin-orion

Quick review pass + follow up to Mike's question

cirq-core/cirq/ops/controlled_gate.py

95-martin-orion · 2022-03-03T19:06:33Z

cirq-core/cirq/ops/controlled_gate.py

-            tuple((val,) if isinstance(val, int) else tuple(sorted(val)) for val in control_values),
-        )
+        print('building from', control_values)
+        self.control_values = cv.ControlValuesBuilder().append(control_values).build()


RE @MichaelBroughton's comment: my concern is how this completely replaces existing control behavior. qsim has optimizations for simple controls (|0> or |1>), but I'd need to check with Sergei on whether they're generalizable to arbitrary controls.

Outside of that concern, though, qsim is happy as long as the gate can be expressed as a unitary on at most 6 qubits total (the optimization allows 6 target qubits + any number of controls).

95-martin-orion · 2022-03-03T19:25:02Z

cirq-core/cirq/ops/control_values.py

+    _combinations: Any
+    _nxt: Optional['ControlValues'] = None


What is the motivation behind this linked-list structure, instead of e.g. a list-of-lists? It seems to me that something like this would be more manageable:

[[comb1, comb2, ...], [nxt_comb1, nxt_comb2, ...], ...]

this assumes that lists of combinations are the only way that wil be used to define control_values, there was a previous comment on this PR about possibly supporting other ways like tensors or sympy expressions in the future.

the linked-list structure allows defining using different ways at the same time e.g. CV1(a table) -> CV2(a sympy expression) -> ... etc

each of those object will inheret from the ControlValues base class so that wil work perfectly.

The comment referred to above is on the linked issue. #4512 (comment)

Another thought, have you considered making a separate ControlValuesProduct class that merges two (or maybe N) ControlValues objects into a product? That approach would allow for a smaller base class and adhere more to the single responsibility pattern.

95-martin-orion · 2022-03-03T19:29:42Z

cirq-core/cirq/ops/controlled_operation.py

+        for control_vals in self.control_values:
+            control_vals = cv.ControlValues.flatten(control_vals)


At a glance, this doesn't appear to add functionality - for a control like |00><00| + |11><11| I would expect some extra handling to be needed (sum over tensors?), and tests to be added to cover it.

Is this being added in a future PR?

the |00><00| + |11><11| combination can be added in the same way that the xor combination |01><01| + |10><10| which was not supported in the old way but is supported now as can be seen in the tests

Did we ever have a design review on this one? I provided the initial code reviews assuming a design was approved, but I still kind of stand by what I originally said on the linked issue, that this could be done more simply by just adding a bool flag to the ControlledGate constructor. That flag would control whether to interpret the 2D input array vertically as allowed states by qubit (which is how it currently works, but is limited), or horizontally as a full set of allowed states (which allows full control).

That this question came up during code review makes me think end users are going to have trouble understanding how to use these classes.

I introduced the idea in #4512 (comment) and talked about it ina cirq meeting #4512 (comment), wrote a prototype in #4512 (comment) and got a review in #4512 (comment)

the boolean idea has some issues

it doesn't address your previous comment Controlled gate construction cannot construct some cases #4512 (comment), it fact it will make it harder to add those new ways lalter.

there is no way of getting around computing products, what a boolean will do is to force the users to compute them before passing them to the constructor, a work around would be passing an array of booleans but that has its own issues.

any work around will make the constructors for controlledGate and controlledOperation execture complex logic which is not a good idea in general, introduce tech debt and make it harder change or support new types later,

lets discuss this in the next cirq meeting.

Sounds good. I have limited availability for cynqs these days, but I think a reprise with the rest of the team on Wed would be great just to get the ball rolling on this PR, since it seems to keep going stagnant. I'm looking forward to the functionality.

NoureldinYosri

I'll remove the debug line and add the classes to the root namespace ASAP.

the main point of the link-list structure is all for different ways of creating control_values (e.g. the old way, the new way, using tensors, using generators, using sympy expressions ..etc) to be used together.

95-martin-orion · 2022-03-17T19:03:15Z

Bump: the checks appear to have gotten stuck 😖 . @NoureldinYosri, you may need to push a change (maybe resolving the conflicts marked) to get it moving again.

daxfohl

Added a couple of thoughts that we could go over during cynq.

daxfohl · 2022-03-20T15:11:34Z

cirq-core/cirq/ops/control_values.py

+    _combinations: Any
+    _nxt: Optional['ControlValues'] = None


Another thought, have you considered making a separate ControlValuesProduct class that merges two (or maybe N) ControlValues objects into a product? That approach would allow for a smaller base class and adhere more to the single responsibility pattern.

daxfohl · 2022-03-20T15:15:36Z

cirq-core/cirq/ops/control_values.py

+
+
+@ControlValues.register
+class SimpleControlValues(ControlValues):


Let's bikeshed this during synq. I don't think users will find the name intuitive. Perhaps PerQubitControlValues? Also the docstring could use some examples to make it clear.

daxfohl

One more question

daxfohl · 2022-03-20T15:48:08Z

cirq-core/cirq/ops/controlled_gate.py

            return (
                f'cirq.ControlledGate(sub_gate={self.sub_gate!r}, '
                f'num_controls={self.num_controls()!r})'
            )
        return (
            f'cirq.ControlledGate(sub_gate={self.sub_gate!r}, '
-            f'control_values={self.control_values!r},'
+            f'control_values={self.control_values.arrangements()!r},'


Does this still round-trip? Given there's no type information about which subclass of ControlValues it uses, I'm not seeing how it could.

NoureldinYosri · 2022-03-21T13:46:46Z

@daxfohl @95-martin-orion I have a simple way of implementing this which I think addresses all the comments/concerns, I will present it during the cirq meeting for discussion.

daxfohl · 2022-03-21T15:05:03Z

Great! One other thing to look out for is the circuit diagram. The current logic labels each qubit with the control values. That probably won't work for the new scenarios. Will need a different approach for them.

…

On Mon, Mar 21, 2022, 6:46 AM Noureldin ***@***.***> wrote: @daxfohl <https://github.com/daxfohl> @95-martin-orion <https://github.com/95-martin-orion> I have a simple way of implementing this which I think addresses all the comments/concerns, I will present it during the cirq meeting. — Reply to this email directly, view it on GitHub <#4714 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG464PGUDURYJ2V5NQRFTLVBB4VDANCNFSM5JCVGBLA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

95-martin-orion · 2022-03-23T18:23:50Z

Discussed on Cirq sync: @NoureldinYosri will write a document capturing the overarching design of this change, we'll discuss and reach a consensus there, then any changes can be reflected here.

NoureldinYosri · 2022-03-29T14:35:47Z

As discussed in the cirq-sync meeting I wrote a design descriping the current approach.

@tanujkhattar @95-martin-orion @daxfohl @dabacon

tanujkhattar · 2022-07-18T22:42:40Z

This can now be closed after #5788

NoureldinYosri requested review from cduck, vtomole and a team as code owners November 30, 2021 19:47

google-cla bot added the cla: yes Makes googlebot stop complaining. label Nov 30, 2021

NoureldinYosri requested a review from dabacon November 30, 2021 19:47

CirqBot added the size: M 50< lines changed <250 label Nov 30, 2021

NoureldinYosri force-pushed the control_vals branch from 383ec2b to 4a4bab9 Compare November 30, 2021 23:57

NoureldinYosri force-pushed the control_vals branch 11 times, most recently from 194915a to 0c6f81e Compare December 1, 2021 01:42

tanujkhattar self-assigned this Dec 2, 2021

NoureldinYosri force-pushed the control_vals branch 9 times, most recently from 2763fdd to 26ec3b7 Compare December 2, 2021 21:02

NoureldinYosri force-pushed the control_vals branch from 0dc16b4 to fe8494b Compare December 11, 2021 14:05

dabacon reviewed Dec 15, 2021

View reviewed changes

NoureldinYosri force-pushed the control_vals branch from fe8494b to 320511c Compare December 18, 2021 13:37

NoureldinYosri mentioned this pull request Jan 17, 2022

Controlled gate construction cannot construct some cases #4512

Closed

daxfohl suggested changes Jan 17, 2022

View reviewed changes

daxfohl reviewed Jan 17, 2022

View reviewed changes

cirq-core/cirq/ops/control_values.py Outdated Show resolved Hide resolved

daxfohl suggested changes Jan 29, 2022

View reviewed changes

cirq-core/cirq/ops/control_values.py Outdated Show resolved Hide resolved

cirq-core/cirq/ops/control_values.py Outdated Show resolved Hide resolved

cirq-core/cirq/ops/control_values.py Outdated Show resolved Hide resolved

cirq-core/cirq/ops/control_values.py Outdated Show resolved Hide resolved

95-martin-orion requested changes Mar 3, 2022

View reviewed changes

NoureldinYosri commented Mar 3, 2022

View reviewed changes

added classes in control_values.py to root namespace

42d7aad

daxfohl reviewed Mar 20, 2022

View reviewed changes

tanujkhattar closed this Jul 18, 2022

		return tuple(tuple(product) for product in vals)


		class ControlValues:

		for control_vals in self.control_values:
		control_vals = cv.ControlValues.flatten(control_vals)



		@ControlValues.register
		class SimpleControlValues(ControlValues):

Created ControlValues for controlled gates/operations, fix for #4512 #4714

Created ControlValues for controlled gates/operations, fix for #4512 #4714

Conversation

NoureldinYosri commented Nov 30, 2021 • edited Loading

tanujkhattar commented Dec 2, 2021

NoureldinYosri commented Dec 11, 2021

dabacon left a comment

Choose a reason for hiding this comment

NoureldinYosri commented Jan 17, 2022

daxfohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxfohl commented Jan 17, 2022

daxfohl left a comment

Choose a reason for hiding this comment

NoureldinYosri commented Jan 28, 2022 • edited Loading

daxfohl commented Jan 29, 2022

NoureldinYosri commented Feb 23, 2022

daxfohl commented Feb 24, 2022

MichaelBroughton commented Mar 3, 2022

95-martin-orion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoureldinYosri Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoureldinYosri left a comment

Choose a reason for hiding this comment

95-martin-orion commented Mar 17, 2022

daxfohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxfohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoureldinYosri commented Mar 21, 2022 • edited Loading

daxfohl commented Mar 21, 2022 via email

95-martin-orion commented Mar 23, 2022

NoureldinYosri commented Mar 29, 2022

tanujkhattar commented Jul 18, 2022

NoureldinYosri commented Nov 30, 2021 •

edited

Loading

NoureldinYosri commented Jan 28, 2022 •

edited

Loading

NoureldinYosri Mar 3, 2022 •

edited

Loading

NoureldinYosri commented Mar 21, 2022 •

edited

Loading