-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Created SumOfProducts subclass of ControlValues #5755
Conversation
This class allows the creation of control values that can't be factored into simple products hence solving quantumlib#4512
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left a couple questions.
nand = SumOfProducts(((0, 0), (0, 1), (1, 0))) # nand of two bits | ||
""" | ||
|
||
_internal_representation: Tuple[Tuple[int, ...], ...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a post init method which verifies that length of all nested tuples in self._internal_representation
is the same? This is assumed to be true in the implementation of methods like _number_variables
below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, I also test for the uniqueness of products in it
cirq-core/cirq/ops/control_values.py
Outdated
return frozenset(self._internal_representation) == {(1,) * self._number_variables()} | ||
|
||
def diagram_repr(self) -> str: | ||
return NotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the diagram_repr not implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the most basic implementation is problematic since the strings will be quite large since they have size num_products x num_qubits => O(num_qubits x 2^num_qubits), this is why I'm hestiant to implement it, however I added it anyway :)
cirq-core/cirq/ops/control_values.py
Outdated
def __getitem__( | ||
self, key: Union[int, slice] | ||
) -> Union['AbstractControlValues', Tuple[int, ...]]: | ||
return NotImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this NotImplemented? Also, I don't like the pattern of
a) Declaring abstract methods in the API, telling the users that all implementations will implement these abstract methods.
b) Returning NotImplemented
in the implementation of the abstract methods.
If we want to allow implementations to return NotImplemented
; then these shouldn't be abstract methods in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason that I'm hesitant to implement it is that it will have a different meaning from that of ProductOfSums, where in ProductOfSums we get valide values of qubit, here we get a product
if not isinstance(other, SumOfProducts): | ||
raise TypeError( | ||
f'And operation not supported between types SumOfProducts and {type(other)}' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not doing the the linked-list style concatenation discussed earlier? Will that come in a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would wait until next PR, I want to introduct the SumOfProducts class before Cirq 1.0, because it has full expressive power (i.e. fixes the original issue), the linked structure can be introducted later without affecting users
the difference between the current state and what will happen when the linked structure is introduced is expressions like this ((x xor y) and (z and w)) and can now be represented by a SumOfProducts objects that has 8 products, while when we introduce the linked structure then we will only need to store 5 products (2 for the first and 3 for the second) and similarly for larger expressions where we would could in some cases store an exponenetial number of products in pseudopolynomial space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doctests are failing because they run the python expression in docstrings written within ">>>" code blocks and compare the expected ouptut with received output.
I've suggested changes that should fix the failing tests. See https://docs.python.org/3/library/doctest.html for more details.
cirq-core/cirq/ops/control_values.py
Outdated
This can be constructed as | ||
>>> xor_control_values = cirq.SumOfProducts(((0, 0), (1, 1))) | ||
>>> q0, q1, q2 = cirq.LineQubit.range(3) | ||
>>> cirq.X(q2).controlled_by(q0, q1, control_values=xor_control_values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> cirq.X(q2).controlled_by(q0, q1, control_values=xor_control_values) | |
>>> xor_cop = cirq.X(q2).controlled_by(q0, q1, control_values=xor_control_values) |
cirq-core/cirq/ops/control_values.py
Outdated
|
||
>>> nand_control_values = cirq.SumOfProducts(((0, 0), (0, 1), (1, 0))) | ||
>>> q0, q1, q2 = cirq.LineQubit.range(3) | ||
>>> cirq.X(q2).controlled_by(q0, q1, control_values=nand_control_values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>>> cirq.X(q2).controlled_by(q0, q1, control_values=nand_control_values) | |
>>> nand_cop = cirq.X(q2).controlled_by(q0, q1, control_values=nand_control_values) |
@tanujkhattar I fixed the docstring and added the optional argument to diagram_repr that we discussed offline |
…moved indexing from abstract class
@tanujkhattar I added the changes we discussed offline namely I did't get to testing ControlledOperations/Gates with SumOfProducts, however it should work fine, however I know of a line that will break (and worse silently) with SumOfProducts and that is this one https://github.com/quantumlib/Cirq/blob/master/cirq-core/cirq/ops/controlled_operation.py#L170 the comparison logic will break I tried a quick fix but it didn't work, I think in a follow up in which we add the tests we discussed we will also need to fix this line |
@NoureldinYosri Thanks for the changes; I'll merge this PR and implement the additional fixes. Have a safe trip! |
This class allows the creation of control values that can't be factored into simple products hence solving quantumlib#4512
This class allows the creation of control values that can't be factored into simple products hence solving #4512