-
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
Refactor AbstractControlValues and it's implementations to fix multiple bugs and improve consistency #5788
Conversation
…le bugs and improve consistency
self._control_qid_shape = tuple(control_qid_shape) | ||
|
||
# Convert to sorted tuples | ||
# Convert to `cv.ProductOfSums` if input is a tuple of control values for each qubit. |
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.
If there's any concern about maintaining the new classes, we could make them private and only use them as internal implementation details. We already do this here with ProductOfSums, and we could do so with SumOfProducts by allowing the control_values param to be a Set[Sequence[int]]
, interpreting that as a ProductOfSums (which is the only natural interpretation) and doing that conversion here too. That way we allow SumOfProducts functionality, but wouldn't have to expose AbstractControlValues in the constructor parameters here, and it would allow us to continue to evolve things after 1.0.
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 we created the abstract class is to use it here and elsewhere wherever control values are used, the abstract functions defined in the abstract class should be the main way for interacting with control values (except in special places, like defining indexing for ProductOfSums)
this way in our code we don't have to do things like 'if isinstance' or to compute on the fly values
as for users they should pass an instance of child class of the abstract class (for now ProductOfSums or SumOfProducts) and everything will work fine, the old way of defining control values is strill defined for backward compatibility but under the hood it transforms it to a ProductOfSums
if TYPE_CHECKING: | ||
import cirq | ||
|
||
|
||
@value.value_equality |
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.
does this still gaurntee immutability?
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.
Exposing properties instead of public member variables is the Cirq's preferred way of "ensuring immutability" in python. If users are mutating private variables directly, it's on them.
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.
This seems reasonable to me. @maffoo do you want to take a look ?
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.
LGTM
I mostly looked at the tests to see the behavior.
Validate that control values are in the half closed interval | ||
[0, qid_shapes) for each qubit. | ||
def __iter__(self) -> Iterator[Tuple[int, ...]]: | ||
"""Iterator on internal representation of control values used by the derived classes. |
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.
This seems dangerous for __iter__
to yield the internal representation, since special dunder methods are part of the public interface of a class. Previously, this called self._expand
so it would always yield tuples from the SumOfProducts
representation, which I think made more sense. If users need access to the internal representation, I think they should directly iterate over the internal fields.
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 agree, but the problem with returning the expanded representation is that it will break all user code of the form
>>> for cv in controlled_gate.control_values:
# cv right now is a iterating on product of sums but will later iterate on sum of products.
This will also be a silent break (no deprecation cycle; no warnings; no API breakages).
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.
Well, any code that is currently iterating expects to get values from _expand()
(e.g. from the SumOfProducts
representation). If we keep __iter__
I think we should stick with that rather than changing to iterate over the internal representation directly.
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.
That change hasn't been released yet. So in the latest released version, the user code expects to iterate on a Tuple[Tuple[int, ...], ...]
representing control values (as a product of sums).
See
Cirq/cirq-core/cirq/ops/controlled_gate.py
Line 125 in f904a09
def control_values(self) -> Tuple[Tuple[int, ...], ...]: |
# 1. Multiple controls for 1 qubit. | ||
# - Duplicates and Permutation across different terms is ignored. | ||
eq.add_equality_group( | ||
cirq.SumOfProducts([0, 1, 2], name="custom name"), |
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.
There's an ambiguity here, whether this should be interpreted as a sum with one 3-qubit conjunction (e.g. ((0, 1, 2),)
), or as a sum with three 1-qubit conjunctions (e.g. ((0,), (1,), (2,))
). I think for SumOfProducts
the former interpretation is much more natural, but the code is doing the latter.
I guess my suggestion would be: if given a 1D object instead of a 2D object, both the SumOfProducts
and ProductOfSums
constructors should add the SUM axis which is variable (its length does not have to be num_qubits
). So:
SumOfProducts([1, 2, 3]) -> SumOfProducts(((1, 2, 3),))
ProductOfSums([1, 2, 3]) -> ProductOfSums(((1,), (2,), (3,)))
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 think this would cause some ambiguity for SumOfProducts
. For example:
>>> SumOfProducts([1, 2, 3]) != SumOfProducts([3, 2, 1])
>>> SumOfProducts([[1], [2], [3]]) == SumOfProducts([[3], [2], [1]])
The current approach is to assume that the missing axis is always the "inner" axis; so in SumOfProducts
we add a the product
axis and in ProductOfSums
we add the sum
axis; which avoids inconsistencies of the form described above.
It also feels natural to me to look at an object and start indexing from the outer-most axis to the inner-most axis; and hence the i'th term of a 1D sum of products representation should represent a "product"; just like it's corresponding 2d version would.
Do you know of any other such examples in standard libraries where the newly added axis can be inner or outer depending on context / implementation ?
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.
Well, in most cases I'd say APIs typically add outer axes rather than inner ones, for example this is how numpy broadcasting works (the innermost axes are lined up and then extra outer axes of length 1 are added on the lower-dimensional array to match the higher dimensional one), or a function that expects a sequence lets you specify one item and it will wrap it in a list.
But I still think in this specific scenario it'd make sense to say that we will add the sum axis if needed since that is the one that is "variable" length in that it could apply to the same gate with the same number of qubits and we're just changing the logical predicate. I don't think there's ambiguity with either way of doing things as long as we specify how the constructor works in the docstring (enough to make clear why SumOfProducts([1, 2, 3]) != SumOfProducts([3, 2, 1])
, which is also the case for ProductOfSums
btw).
Another option would be to disallow passing in 1D values to the SumOfProducts
constructor, which I think is how it currently works, and then we could continue the discussion and add that in the future. Then we don't have to hold up the release to decide this.
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.
One more thing I'd like to point out is that the API is Sequence[Union[int, Collection[int]]]
and not Union[Sequence[int], Sequence[Collection[int]]
. This API also conveys that "If you specify an int; we'll wrap it in a list".
In the latter case, we can clearly define that if you provide a 1D object; we'll add an axes for SUM by default; but in the former case I'm not sure what the definition should be.
For example, How should we interpret SumOfProducts([1, [2], [3]])
if we always add an axes for SUM? Currently, it will be interpreted as SumOfProducts([[1], [2], [3]])
.
Do you suggest that we should change the API to Union[Sequence[int], Sequence[Collection[int]]
and raise an error if we encounter the case like above?
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.
Do you suggest that we should change the API to Union[Sequence[int], Sequence[Collection[int]] and raise an error if we encounter the case like above?
Yes, that's what I'd suggest. Though I'd probably change the type annotation to Union[Sequence[int], Collection[Sequence[int]]
since order matters on the inner "product" axis, but not on the outer "sum" axis.
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.
Hmm; this problem doesn't arise with ProductOfSums
, because the Sum axis is also the inner axis. For consistency, it would make sense to also change the API of ProductOfSums
and bring the union outside; but that would mean changing the API of all the public methods which used to accept Sequence[Union[int, Collection[int]]]
historically when we only had product of sums; which I don't think we can / should do at this point.
So; ProductOfSums
would still have Sequence[Union[int, Collection[int]]]
but SumOfProducts
would be Union[Sequence[int], Collection[Sequence[int]]
?
I personally still like the "if you specify an int; we'll wrap it in a list" convention a lot more (and hence keeping things the way they are) but if you have feel very strongly that we should change; I'd be happy to update to the final proposal above (SumOfProducts and ProductOfSums having different API; one supporting inputs of the form [1, [2], [3]]
and other raising an error).
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 feel pretty strongly, but I also think this is something that could be added post-1.0 and for now we could keep the implementation of SumOfProducts
which is documented as accepting 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.
Discussed on gchat, we're going to go with Collection[Sequence[int]]
for now, so has to be specified as 2D. We can discuss making this more flexible in the future.
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.
Updated the API. Also; we use a sorted tuple instead of frozenset for SumOfProducts
internal representation because we want a consistent order of terms in the circuit diagram when plotting controlled operations with sum of products control values.. We could sort the frozen set each time we want to plot the diagram; but that would be wasteful.
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.
LGTM!
…le bugs and improve consistency (quantumlib#5788) * Remove stale TODO * Refactor AbstractControlValues and it's implementations to fix multiple bugs and improve consistency * Revert unrelate change in consistent_protocols * Fix tests and update json * Add more json * Add a lot more tests to control_values_test.py * Address feedback from maffoo@ * Fix pylint and mypy errors * maffoo@ feedback part-2 * Update diagrams and change SumOfProducts API to Collection[Sequence[int]]
This PR mainly cleans up the newly introduced
AbstractControlValues
interface it'sSumOfProducts
andProductOfSums
implementations. The following is a summary of major changes and design choices that have been made as part of this PR, which users / reviewers should be aware of.1. Controlled gate and operation now return an instance of
AbstractControlValues
instead ofTuple[Tuple[int, ...], ...]
This is important because every decision we make about the
AbstractControlValues
implementation can impact / break all user code usingcontrolled_gate.control_values
or controlled_operation.control_values`2.
ProductOfSums
will now always returnFalse
in an equality check withTuple[Tuple[int, ...], ...]
.3. Iterating on an instance of
AbstractControlValues
has a different meaning forProductOfSums
andSumOfProducts
implementationsSumOfProducts
will give the i'th expanded term, having one bit for each controlled qubit; whereas iterating onProductOfSums
will give the tuple of control values for which the i'th qubit should be activated.for value in controlled_gate.control_values
; which is a pretty big breaking change without a deprecation cycle. With this decision; we preserve backwards compatibility and still provide users a way to dofor value in controlled_values.expand()
if they want a consistent behavior for every instance ofAbstractControlValues
.len(control_values)
does not exist and users should instead docirq.num_qubits(control_values)
; becauselen
can be ambiguous (number of qubits or number of terms in the internal representation?)4. str and diagram representations for the SumOfProducts have been updated
ProductOfSums
andSumOfProducts
are distinguishable.cc @NoureldinYosri @MichaelBroughton @daxfohl @dabacon @maffoo