Fix most numpy errors in cirq/qis and friends #4002
Conversation
cirq/qis/measures.py
Outdated
| @@ -13,7 +13,7 @@ | |||
| # limitations under the License. | |||
| """Measures on and between quantum states and operations.""" | |||
|
|
|||
| from typing import Optional, TYPE_CHECKING, Tuple | |||
| from typing import Optional, TYPE_CHECKING, Tuple, cast | |||
There was a problem hiding this comment.
Optional: alphabetic ordering?
cirq/qis/measures.py
Outdated
| state2_arr = ( | ||
| state2.density_matrix() | ||
| if state2._is_density_matrix() | ||
| else cast(np.ndarray, state2.state_vector()) |
There was a problem hiding this comment.
Optional: IIUC this is just to convert from Optional[np.ndarray] to np.ndarray. It'd be nice if you could make it more apparent to the reader. Perhaps an assert is understood by both mypy and humans?
There was a problem hiding this comment.
assert would work but not in this ternary operation
I found this thread when going through these: python/typing#645
honestly, I think these code patterns indicate that there's a missing method. Either "state_vector_for_sure_because_i_already_checked" or array_form_irrespective_of_whether_its_density_or_state. I'll investigate
There was a problem hiding this comment.
Otherwise the cast is safe since the only time state_vector returns none is when _is_density_matrix is true. Obviously mypy isn't smart enough to deduce that full logic
There was a problem hiding this comment.
I agree with you that there is a missing function to aid this pattern. Perhaps something like Rust's unwrap? This would have the benefit of being useful for all Optional types, not just state vectors. Ideally, we'd use it similar to how you're using cast here:
state2_arr = (
state2.density_matrix()
if state2._is_density_matrix()
else unwrap(state2.state_vector())WDYT?
There was a problem hiding this comment.
Please check out the latest change, namely 45ed207 which fixes these the right way ™️
There was a problem hiding this comment.
I like it.
nit: maybe state_or_density_array -> state_vector_or_density_matrix. The latter is clearer (especially since state_tensor is also a thing so state alone in this context is ambiguous) and more consistent with other method names.
There was a problem hiding this comment.
sure. I was trying to think of names that weren't too long but I guess this is a somewhat niche function so it can handle a long name.
edit: esp. since there are state vectors and tensors in this class
cirq/qis/states.py
Outdated
| @@ -135,7 +136,7 @@ def density_matrix(self) -> np.ndarray: | |||
| (a two-dimensional array). | |||
| """ | |||
| if not self._is_density_matrix(): | |||
| state_vector = self.state_vector() | |||
| state_vector = cast(np.ndarray, self.state_vector()) | |||
There was a problem hiding this comment.
ditto (optional as above, please address neither or both for consistency)
cirq/qis/states.py
Outdated
| @@ -159,7 +160,10 @@ def validate( | |||
| is_state_tensor = self.data.shape == self.qid_shape | |||
| if is_state_vector or is_state_tensor: | |||
| validate_normalized_state_vector( | |||
| self.state_vector(), qid_shape=self.qid_shape, dtype=dtype, atol=atol | |||
| cast(np.ndarray, self.state_vector()), | |||
| @@ -12,7 +12,7 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| import warnings | |||
| from typing import Any, List, Sequence | |||
| from typing import Any, List, Sequence, Optional | |||
There was a problem hiding this comment.
on the basis that none of the imports are ordered in cirq and we don't have tooling to check for or support these, I'm going to not do any alphabetizing of imports :)
There was a problem hiding this comment.
Fair enough in general.
Maybe at least keep the alphabetic order where it pre-exists, e.g. here, and do whatever elsewhere?
There was a problem hiding this comment.
discussed elsewhere, I'm going to hold out for tooling to do this for me
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Part of #3767