Refactored and created an abstraction for control values

[NoureldinYosri]

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

[NoureldinYosri]

@tanujkhattar

[tanujkhattar]

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?

[NoureldinYosri]

yes, this is just for refactoring, this function will be rewritten in a better way once the linked structure is introduced

[tanujkhattar]

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?

[NoureldinYosri]

@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.

[tanujkhattar]

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?

[NoureldinYosri]

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

[tanujkhattar]

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?

[tanujkhattar]

nit:

Suggested change

	def __iter__(self) -> Generator[Tuple[int, ...], None, None]:
	def __iter__(self) -> Iterator[[Tuple[int, ...]]:

[NoureldinYosri]

this is blocked by mypy, it insists on Generator, I originally tried to use Iterator but had to use Generator instead

[tanujkhattar]

The docstring should be more detailed. For example:

Suggested change

	"""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.
	"""

[NoureldinYosri]

PTAL

[tanujkhattar]

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?

[NoureldinYosri]

PTAL

[tanujkhattar]

LGTM % a few final nits, mainly around docstrings. Will be happy to merge once they are resolved.

[tanujkhattar]

nit: Either convert to a one-liner docstring or follow the guidance for multi-line docstrings described above.

Suggested change

	"""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."""