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.
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.
yes, this is just for refactoring, this function will be rewritten in a better way once the linked structure is introduced
|
@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.
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.
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.
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.
nit:
| def __iter__(self) -> Generator[Tuple[int, ...], None, None]: | |
| def __iter__(self) -> Iterator[[Tuple[int, ...]]: |
There was a problem hiding this comment.
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.
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. | |
| """ |
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.
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?
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.
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.""" |
tanujkhattar
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
…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