Modify some numpy types to be compatible with numpy>=1.20 - #5668
Conversation
dstrain115
left a comment
dstrain115
left a comment
There was a problem hiding this comment.
One comment, otherwise seems fine to me.
|
|
||
| def measure( | ||
| self, axes: Sequence[int], seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' = None | ||
| self, axes: Sequence[int], seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' |
There was a problem hiding this comment.
Are we sure that removing the default won't break anything? Is it possible anyone is calling measure() without specifying a seed? (Same for below).
There was a problem hiding this comment.
I've put the default back but set it to np.random instead of None because it's needed by _measure.
| qubits: Tuple['cirq.Qid', ...], | ||
| dtype: Type[np.complexfloating], | ||
| ) -> np.ndarray: | ||
| ) -> Optional[np.ndarray]: |
There was a problem hiding this comment.
Please revert, the return value on line 2627 can be only None if protocols.apply_unitaries is called with default=None; without search argument it raises TypeError. #5669
| """Utility methods for combining matrices.""" | ||
|
|
||
| import functools | ||
| from typing import Union, TYPE_CHECKING |
There was a problem hiding this comment.
nit: abc imports (also below)
There was a problem hiding this comment.
Not needed anymore. Reverted.
| The kronecker product of all the inputs. | ||
| """ | ||
| product = np.ones(shape=(1,) * shape_len) | ||
| product: np.ndarray[ |
There was a problem hiding this comment.
I think this can just be np.ndarray (worked locally for me). The extra typing likely is just more confusing than helpful.
| state = qis.eye_tensor(qid_shape, dtype=dtype) | ||
|
|
||
| result = _apply_unitary_circuit(self, state, qs, dtype) | ||
| assert result is not None |
|
Thanks for tackling these @vtomole . We cleared out a lot of the easy ones, the more fun ones are all that are left! |
|
|
||
| @abc.abstractmethod | ||
| def measure( | ||
| self, axes: Sequence[int], seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' = None |
There was a problem hiding this comment.
slightly strange that this one is None, but. one below for sample is None. Is this coming from a subclass?
There was a problem hiding this comment.
Wanted to be consistent with clifford_tableau's measure but given that it's actually not needed, I've reverted this change.
| def measure( | ||
| self, axes: Sequence[int], seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' = None | ||
| self, axes: Sequence[int], seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' = np.random | ||
| ) -> List[int]: |
There was a problem hiding this comment.
I think I would keep this None and then do a seed or np.random in the parameter in self._measure. That way we don't have to change the default here (though I don't think it will matter much). np.random might be considered mutable(?) since it changes when different people use it, so best to not put mutables as defaults.
There was a problem hiding this comment.
Do we want it as a parameter or like:
def _measure(self, q, prng: Optional[np.random.RandomState]) -> int:
if prng is None:
prng = np.randomThere was a problem hiding this comment.
that could also work, though this also does something I try to avoid which is re-assign a parameter. Mostly this doesn't ever hurt you, but there are cases where it does (mostly having to do with pytest, btw). It also sort of makes sense that the private method has a random state, but the public one has the default of None.
There was a problem hiding this comment.
Ah, makes sense. Switched to param.
vtomole
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
5 participants
Part of #3767