Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More numpy types #5683

Merged
merged 11 commits into from Jul 12, 2022
Merged

More numpy types #5683

merged 11 commits into from Jul 12, 2022

Conversation

vtomole
Copy link
Collaborator

@vtomole vtomole commented Jul 8, 2022

12 errors left

Part of #3767

@CirqBot CirqBot added the size: M 50< lines changed <250 label Jul 8, 2022
@vtomole vtomole marked this pull request as ready for review July 8, 2022 05:53
@vtomole vtomole requested review from mrwojtek, a team and cduck as code owners July 8, 2022 05:53
@vtomole vtomole requested a review from maffoo July 8, 2022 05:53
@@ -1592,7 +1592,7 @@ def _control_keys_(self) -> FrozenSet['cirq.MeasurementKey']:


def _overlap_collision_time(
c1: Sequence['cirq.Moment'], c2: Sequence['cirq.Moment'], align: 'cirq.Alignment'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very strange to be using a numpy array to store Moments. I think for this one one needs to go back to the source where the numpy array is created and use a sequence instead (and change signature of concat_ragged function as well)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed buffer = np.zeros(shape=pad_len * 2 + n_acc, dtype=object) to buffer: MutableSequence['cirq.Moment'] = [cirq.Moment()] * (pad_len * 2 + n_acc). Please take another look.

@@ -331,6 +331,7 @@ def _trace_distance_bound_(self) -> Optional[float]:
if protocols.is_parameterized(self._exponent):
return None
angles = np.pi * (np.array(self._eigen_shifts()) * self._exponent % 2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra debug print?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -644,7 +644,7 @@ def expectation_from_density_matrix(
*,
atol: float = 1e-7,
check_preconditions: bool = True,
) -> float:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should remain float. The bug is that in numpy.trace below, which should just be unpacked into a float. line 746. Does that fix things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've returned float(result * self.coefficient) since np.trace returns a sum.

"""Performs a projective measurement on the q'th qubit.

Returns: the result (0 or 1) of the measurement.
"""
real_prng: np.random.RandomState = (
random_state.parse_random_state(np.random) if prng is None else prng
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this can be simplied to prng or random_state.parse_random_state(np.random)

"""Measures the q'th qubit.

Reference: Section 4.1 "Simulating measurements"

Returns: Computational basis measurement as 0 or 1.
"""
real_prng: np.random.RandomState = (
random_state.parse_random_state(np.random) if prng is None else prng
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar simplification as before

@@ -226,7 +226,7 @@ def prepare_into_buffer(k: int):
]
p = prng.random()
weight = None
fallback_weight = 0
fallback_weight = np.float64(0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does 0.0 work instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get

cirq-core/cirq/sim/state_vector_simulation_state.py:228: error: Incompatible types in assignment (expression has type "float", variable has type "floating[Any]") [assignment] when we do this. I can either leave this as-is or do 0.0 here and weight = float(np.linalg.norm(self._buffer) ** 2) a little bit more down.

@@ -58,7 +59,9 @@ def random_qubit_unitary(
rng: Random number generator to be used in sampling. Default is
numpy.random.
"""
real_rng: np.random.RandomState = np.random if rng is None else rng
real_rng: np.random.RandomState = (
random_state.parse_random_state(np.random) if rng is None else rng
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplify

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -331,6 +331,7 @@ def _trace_distance_bound_(self) -> Optional[float]:
if protocols.is_parameterized(self._exponent):
return None
angles = np.pi * (np.array(self._eigen_shifts()) * self._exponent % 2)
print(type(angles))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@@ -716,7 +716,7 @@ def expectation_from_density_matrix(

def _expectation_from_density_matrix_no_validation(
self, state: np.ndarray, qubit_map: Mapping[TKey, int]
) -> float:
) -> np.ndarray:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value of this function is scalar. After adding a debug printout the pauli_string_test.py shows it returns complex values with a zero imaginary part, but PauliString.coefficient is typed as complex.

Is the expectation value guaranteed to be a float?
Please convert the return value to a complex or float as suitable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've converted it to a float.

"""Performs a projective measurement on the q'th qubit.

Returns: the result (0 or 1) of the measurement.
"""
real_prng: np.random.RandomState = (
random_state.parse_random_state(np.random) if prng is None else prng
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_random_state already returns np.random for None.

Suggested change
random_state.parse_random_state(np.random) if prng is None else prng
random_state.parse_random_state(prng)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Applied.

"""Measures the q'th qubit.

Reference: Section 4.1 "Simulating measurements"

Returns: Computational basis measurement as 0 or 1.
"""
real_prng: np.random.RandomState = (
random_state.parse_random_state(np.random) if prng is None else prng
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this method. _measure() is only called from the measure() method which can receive an integer seed argument. Let's use parse_random_state() there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for this and cliiford_tableau above since the concepts are similar.

@@ -386,7 +389,7 @@ def apply_global_phase(self, coefficient: value.Scalar):
self.omega *= coefficient

def measure(
self, axes: Sequence[int], seed: 'cirq.RANDOM_STATE_OR_SEED_LIKE' = None
self, axes: Sequence[int], seed: Optional['cirq.RANDOM_STATE_OR_SEED_LIKE'] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, keep the seed type as it was, RANDOM_STATE_OR_SEED_LIKE is typed as Any so None should be fine there. Add a parse_random_state() call so that self._measure() gets a correct type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The seed type is assigned to None so it's still Optional. This is being explicit even though we haven't turned on no-implicit-optional.

Copy link
Collaborator Author

@vtomole vtomole Jul 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't confirmed that

VarType = Any
def hi(var: VarType=None)

Fails with no-implicit-optional though.

Anyway, will apply your suggestion since Any is vague so mypy shouldn't have a problem with the previous type.

Comment on lines 143 to 152
def _copy_density_matrix_to_out(density_matrix: np.ndarray, out: np.ndarray) -> np.ndarray:
np.copyto(dst=out, src=density_matrix)
return out

arrout: np.ndarray = (
np.copy(density_matrix)
if out is None
else density_matrix
if out is density_matrix
else (np.copyto(dst=out, src=density_matrix), out)[-1]
else _copy_density_matrix_to_out(density_matrix, out)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider replacing with hopefully more readable

    arrout: np.ndarray
    if out is None:
        arrout = np.copy(density_matrix)
    elif out is density_matrix:
        arrout = density_matrix
    else:
        np.copyto(dst=out, src=density_matrix)
        arrout = out

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -226,7 +226,7 @@ def prepare_into_buffer(k: int):
]
p = prng.random()
weight = None
fallback_weight = 0
fallback_weight = np.float64(0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fallback_weight = np.float64(0)
fallback_weight = 0.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get

cirq-core/cirq/sim/state_vector_simulation_state.py:228: error: Incompatible types in assignment (expression has type "float", variable has type "floating[Any]") [assignment] when we do this. I can either leave this as-is or do 0.0 here and weight = float(np.linalg.norm(self._buffer) ** 2) a little bit more down.

Comment on lines 62 to 64
real_rng: np.random.RandomState = (
random_state.parse_random_state(np.random) if rng is None else rng
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace with a simple call to parse_random_state()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 96 to 101
values_range = np.arange(len(values))
_ = (
ax.bar(values_range, values, tick_label=values_range)
if tick_label is None
else ax.bar(values_range, values, tick_label=tick_label)
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mypy complaint was

error: Incompatible types in assignment (expression has type "ndarray[Any, dtype[signedinteger[Any]]]", variable has type "Optional[Sequence[str]]") [assignment]

Let's replace with

Suggested change
values_range = np.arange(len(values))
_ = (
ax.bar(values_range, values, tick_label=values_range)
if tick_label is None
else ax.bar(values_range, values, tick_label=tick_label)
)
if tick_label is None:
tick_label = [str(i) for i in range(len(values))]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vtomole vtomole added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 12, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 12, 2022
@vtomole vtomole dismissed pavoljuhas’s stale review July 12, 2022 06:22

requested changes addressed.

@CirqBot CirqBot merged commit 89a5323 into quantumlib:master Jul 12, 2022
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 12, 2022
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 12, 2022
@vtomole vtomole deleted the numpy branch July 12, 2022 06:37
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants