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.

Sign up for GitHub

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

Jump to bottom

Fix more mypy --next type errors #5392

Merged
merged 6 commits into from
May 24, 2022
Merged

Fix more mypy --next type errors #5392

merged 6 commits into from
May 24, 2022

Conversation

dabacon
Copy link
Collaborator

@dabacon dabacon commented May 23, 2022

Run against check/mypy --next

See #3767

@dabacon dabacon requested review from a team, vtomole and cduck as code owners May 23, 2022 20:27
@dabacon dabacon requested a review from 95-martin-orion May 23, 2022 20:27
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@CirqBot CirqBot added the size: M 50< lines changed <250 label May 23, 2022
95-martin-orion
95-martin-orion reviewed May 23, 2022
View reviewed changes
Copy link
Collaborator

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

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

Using cast to fix these doesn't feel much better than using type: ignore. Since they're numeric conversions, could we instead do <type>(<var>) to enforce type?

@dabacon
Copy link
Collaborator Author

dabacon commented May 23, 2022 •
edited
Loading

Using cast to fix these doesn't feel much better than using type: ignore. Since they're numeric conversions, could we instead do <type>(<var>) to enforce type?

For the resolver case: the issue with <type>(<var>) in the case of the parameter resolver, is that the relevant type is complex, and so this potentially will change the actual representing type, and I don't think this is what people want. The cast is a no-op, and only helps because numpy doesn't know how to deal with numbers.Complex and complex, nor does it know how to deal with sympy.Expr, even though np.power works correctly in that case (by delegating to __pow__). I think the cast is slightly better that # type: ignore in that it is at least expressing the type that is expected to work, so downstream changes can reason about it.

But for the others, I think you are correct. Updated

@dabacon dabacon merged commit 267c2ee into quantumlib:master May 24, 2022
augustehirth pushed a commit to augustehirth/Cirq that referenced this pull request May 27, 2022
@dabacon @augustehirth
Fix more mypy --next type errors (quantumlib#5392)
2a960b0
mpharrigan
mpharrigan reviewed Jun 22, 2022
View reviewed changes
cirq-core/cirq/transformers/heuristic_decompositions/gate_tabulation_math_utils.py
@@ -58,17 +58,17 @@ def random_qubit_unitary(
rng: Random number generator to be used in sampling. Default is
numpy.random.
"""
rng = np.random if rng is None else rng
real_rng: np.random.RandomState = 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.

we should tell mypy to allow_redefined #3832

mpharrigan
mpharrigan reviewed Jun 22, 2022
View reviewed changes
cirq-core/cirq/vis/state_histogram.py
@@ -92,7 +92,7 @@ def plot_state_histogram(
elif isinstance(data, collections.Counter):
tick_label, values = zip(*sorted(data.items()))
else:
values = data
values = np.array(data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

prefer np.asarray

rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
@dabacon @rht
Fix more mypy --next type errors (quantumlib#5392)
1dc5ede
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
@dabacon
Fix more mypy --next type errors (quantumlib#5392)
4a369e2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpharrigan mpharrigan mpharrigan left review comments

@95-martin-orion 95-martin-orion 95-martin-orion approved these changes

@vtomole vtomole Awaiting requested review from vtomole

@cduck cduck Awaiting requested review from cduck

Assignees
No one assigned
Labels
size: M 50< lines changed <250
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dabacon @mpharrigan @95-martin-orion @CirqBot