Fix most numpy type errors in cirq/linalg #4000
Conversation
Using `check/mypy --next | grep cirq/linalg` this fixes all the problems.
cirq/linalg/decompositions.py
Outdated
| interaction_matrix(y_mat, y), | ||
| interaction_matrix(x_mat, x), | ||
| before, | ||
| return self.global_phase * cast( |
There was a problem hiding this comment.
ditto
(Possibly could apply to other use of cast below, too.)
There was a problem hiding this comment.
changed this per above. what other use of cast?
There was a problem hiding this comment.
Well, there are six more left at this point. It'd be nice to get rid of as many as you can. They may hide bugs and make code harder to read.
cirq/linalg/diagonalize.py
Outdated
| left = left_adjust.T.dot(base_left.T) | ||
| right = base_right.T.dot(right_adjust.T) | ||
| left = cast(np.ndarray, left_adjust.T.dot(base_left.T)) | ||
| right = cast(np.ndarray, base_right.T.dot(right_adjust.T)) |
There was a problem hiding this comment.
So only the cast on left is necessary and I'm trying to figure out why ......
There was a problem hiding this comment.
it seems like in the general case
x: np.ndarray
y: np.ndarray
x.dot(y) --> Union[number[Any], ndarray]but
np.dot(x, y) --> ndarrayweird
There was a problem hiding this comment.
the former is defined in the top-level __init__.pyi type stub file. The latter may be due to the docstring of the skeleton implementation in numpy/core/multiarray.py (that dispatches to c ufuncs). Maybe an oversight on numpy's part? I still don't know why right doesn't need a cast in the above.
There was a problem hiding this comment.
I think the former is the correct signature but the latter is the easier signature. I've re-written this function to use cirq.dot and np.dot to keep everything as ndarrays
cirq/linalg/transformations.py
Outdated
| @@ -326,6 +324,14 @@ def partial_trace(tensor: np.ndarray, keep_indices: List[int]) -> np.ndarray: | |||
| return np.einsum(tensor, left_indices + right_indices) | |||
|
|
|||
|
|
|||
| class EntangledStateError(ValueError): | |||
| """Raised by `sub_state_error` if the result of factoring is not a pure state | |||
There was a problem hiding this comment.
"not a pure state" -> "not a product state"
Also, I'd remove "by sub_state_error" since the exception may be used in future by related functions.
Suggestion:
"""Raised when a product state is expected, but an entangled state is provided."""There was a problem hiding this comment.
Changed to use your nice wording
cirq/linalg/diagonalize.py
Outdated
| raise ValueError('mat1 @ mat2.T must be symmetric.') | ||
| if not predicates.is_hermitian(mat1.T.dot(mat2), rtol=rtol, atol=atol): | ||
| if not predicates.is_hermitian(cast(np.ndarray, mat1.T.dot(mat2)), rtol=rtol, atol=atol): |
There was a problem hiding this comment.
Optional: These are the last two casts remaining. It'd be great if we could get rid of them, too.
There was a problem hiding this comment.
got rid of 'em!
We'll have to keep an eye out if the numpy folks change the signature for np.dot vs. ndarray.dot. It strikes me as odd that they're different.
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
|
Automerge cancelled: A status check is failing. |
CirqBot
removed
automerge
|
test fails on CI and one of my python environments but not the other 🤷 Changing back to |
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Using
check/mypy --next | grep cirq/linalgthis fixes all theproblems.
#3767