-
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
Refactored and created an abstraction for control values #5362
Conversation
…n that replicates current behaviour. refactored the code for control_values and abstracted its calls this is a first step towards implementing more ways to represent control values in order to solve quantumlib#4512
cirq-core/cirq/ops/control_values.py
Outdated
Returns: | ||
An object that represents the cartesian product of the two inputs. | ||
""" | ||
return type(self)(self._internal_representation + other._internal_representation) |
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 assumes that self._internal_representation
and other._internal_representation
can be simply added together. But their types are Any
, so this need not be true.
Does the current implementation assume that ProductOfSums
is the only derived type, and would be special cased later once we add more derived types?
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.
yes, this is just for refactoring, this function will be rewritten in a better way once the linked structure is introduced
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.
As per our last discussion offline, can we strip down the API to have only what's necessary for this particular PR and add the additional methods in the follow up PR?
@tanujkhattar which is why I removed the _iterator function, but all other functions are necessary and are being used/called either directly by functions in controlled_gate, controlled_operation, common_gate or indirectly by other tests. |
cirq-core/cirq/ops/control_values.py
Outdated
|
||
@abc.abstractmethod | ||
def identifier(self) -> Tuple[Any]: | ||
"""Returns an identifier from which the object can be rebuilt.""" |
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.
Improve docstrings, it's not clear to me what it means to "have an identifier from which object can be rebuilt". Does this mean an identifier we can use for serialization / deserialization ?
Also, when would a user use this method? Do we need this to be part of the public API ? Can we just enforce that the class should have a __repr__
defined that satisfies eval(repr(cv)) == cv
?
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 changed the docstring, this function essentially returns the internal representation, I changed it to be private and I will remove it in the next PR, it's now used in functions in controlled_gate and controlled_operation that require access to the internal representation
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.
Left a bunch of comments for cleanup.
One concern I have is that I don't like the use of the private method control_values._identifier()
in controlled gates and operations; because
a) Private methods shouldn't be used outside of the class.
b) it assumes that the underlying _identifier()
has a representation which is factorized -- This is true for ProductOfSums implementation but wouldn't be true in the follow-up PR and I guess we'll have to add isinstance checks and special case the behavior in controlled gates and controlled values on different controlled values implementations?
pass | ||
|
||
def __iter__(self) -> Generator[Tuple[int], None, None]: | ||
def __iter__(self) -> Generator[Tuple[int, ...], None, None]: |
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.
nit:
def __iter__(self) -> Generator[Tuple[int, ...], None, None]: | |
def __iter__(self) -> Iterator[[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.
this is blocked by mypy, it insists on Generator, I originally tried to use Iterator but had to use Generator instead
cirq-core/cirq/ops/control_values.py
Outdated
"""AbstractControlValues is an abstract immutable data class. | ||
|
||
AbstractControlValues defines an API for control values and implements | ||
functions common to all implementations (e.g. comparison). | ||
""" |
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 docstring should be more detailed. For example:
"""AbstractControlValues is an abstract immutable data class. | |
AbstractControlValues defines an API for control values and implements | |
functions common to all implementations (e.g. comparison). | |
""" | |
"""Abstract immutable base class that defines the API for control values. | |
`cirq.ControlledGate` and `cirq.ControlledOperation` are useful to augment existing gates | |
and operations to have one or more control qubits. For every control qubit, the set of | |
integer values for which the control should be enabled is represented by | |
`cirq.AbstractControlValues`. | |
Implementations of `cirq.AbstractControlValues` can use different internal representations | |
to store control values, but they should all satisfy the public API defined here. | |
""" |
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.
PTAL
cirq-core/cirq/ops/control_values.py
Outdated
|
||
@abc.abstractmethod | ||
def _expand(self) -> Iterator[Tuple[Any, ...]]: | ||
"""Returns the control values tracked by the object.""" |
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.
Please explain more what the "expanded" representation means / contains. Does it depend upon what internal representation the subclasses use? Does every tuple correspond to a "one value per qubit" representation?
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.
PTAL
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 % a few final nits, mainly around docstrings. Will be happy to merge once they are resolved.
cirq-core/cirq/ops/control_values.py
Outdated
"""Returns a plain sum of product representation of the values instead | ||
of the (possibly compressed) internal representation. | ||
""" |
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.
nit: Either convert to a one-liner docstring or follow the guidance for multi-line docstrings described above.
"""Returns a plain sum of product representation of the values instead | |
of the (possibly compressed) internal representation. | |
""" | |
"""Expands the (possibly compressed) internal representation into a sum of products representation.""" |
…5362) Created a concrete implementation that replicates current behaviour. refactored the code for control_values and abstracted its calls this is a first step towards implementing more ways to represent control values in order to solve quantumlib#4512
Created a concrete implementation that replicates current behaviour.
refactored the code for control_values and abstracted its calls
this is a first step towards implementing more ways to represent control values in order to solve #4512