Fix sympy 1.10 related mypy typing issues #5263

dstrain115 · 2022-04-15T23:21:47Z

Fixes most of the typing issues related to sympy 1.10
- Most were caused by either:
  - Use of sympy.Basic which is too general (we probably want
    sympy.Expr) that did not have the right duck-typing
  - Not handling of sympy.Expr in various places that could
    theoretically have sympy expressions returned to them.
- Fixed others with casts or other checks.
- About ~10 out of 450 errors I just put type: ignore after struggling
  for a while.

- Fixes most of the typing issues related to sympy 1.10 - Most were caused by either: - Use of sympy.Basic which is too general (we probably want sympy.Expr) that did not have the right duck-typing - Not handling of sympy.Expr in various places that could theoretically have sympy expressions returned to them. - Fixed others with casts or other checks. - About ~10 out of 450 errors I just put type: ignore after struggling for a while.

- Apparently boolean expressions in sympy are not sympy.Expr and are sympy.Basic. Sigh...

dabacon · 2022-04-16T14:18:21Z

cirq-core/cirq/experiments/t1_decay_experiment.py


    if repetitions <= 0:
        raise ValueError('repetitions <= 0')
+    if not isinstance(min_delay_nanos, float) or not isinstance(max_delay_nanos, float):


think this needs to be (float,int) and not just float. isinstance(1, float) returns False.

Also we probably want to allow things like np.int64, np.float64, etc. It might be nice to define a tuple of allowed "numeric" types in one place and use it throughout cirq, though that could be a future PR.

While, in general, this is true, I don't think it is true here, since we control cirq.Duration.total_nanos().
I went ahead and modified cirq.Duration to guarantee returning a float, even when passed a numpy.int64 type.

Actually, I take that back. It looks like Duration is not typed correctly and can return an int. I have made the typing more clear for Duration as well.

dabacon · 2022-04-16T14:18:35Z

cirq-core/cirq/experiments/t2_decay_experiment.py


    # Input validation
    if repetitions <= 0:
        raise ValueError('repetitions <= 0')
+    if not isinstance(min_delay_nanos, float) or not isinstance(max_delay_nanos, float):


Same as above.

dabacon · 2022-04-16T14:21:19Z

cirq-core/cirq/study/resolver.py

@@ -72,7 +72,7 @@ def __init__(self, param_dict: 'cirq.ParamResolverOrSimilarType' = None) -> None

    def value_of(
        self, value: Union['cirq.TParamKey', float], recursive: bool = True
-    ) -> 'cirq.TParamVal':
+    ) -> Union['cirq.TParamVal']:


don't understand this one. Union with what?

maffoo

Thanks for working through all these issues!

A few general comments:

When adding type ignore comments, consider adding the specific error code indicated by mypy, e.g. type: ignore[dict-item].
It would be nice to try to use common type aliases we have already defined like cirq.TParamKey, cirq.TParamVal, and cirq.ParamDictType where possible.
Similarly, it would be nice to define common type tuples to be used in isinstance checks, but this could be something for the future (it's unfortunate that we need to define type unions and tuples separately, though this will be fixed in python 3.10).

maffoo · 2022-04-16T16:08:37Z

cirq-core/cirq/experiments/t1_decay_experiment.py


    if repetitions <= 0:
        raise ValueError('repetitions <= 0')
+    if not isinstance(min_delay_nanos, float) or not isinstance(max_delay_nanos, float):


Also we probably want to allow things like np.int64, np.float64, etc. It might be nice to define a tuple of allowed "numeric" types in one place and use it throughout cirq, though that could be a future PR.

cirq-core/cirq/experiments/xeb_fitting.py

cirq-core/cirq/study/flatten_expressions.py

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

…into sympy_v10_part1

maffoo

Mostly looks good. The only thing I'm not sure about are the circuit_serializer.py tests, which look like a functional change not just a typechecking fix.

cirq-core/cirq/experiments/t1_decay_experiment.py

maffoo · 2022-04-22T18:51:20Z

cirq-core/cirq/experiments/t1_decay_experiment.py

+    min_delay_nanos = min_delay_dur.total_nanos()
+    max_delay_nanos = max_delay_dur.total_nanos()


We might consider adding a function on Duration itself that would get the total nanos as a number and fail if it is symbolic. Then users could call that function instead of doing their own type checks.

Possibly, but that's an issue for another PR.

cirq-core/cirq/experiments/t2_decay_experiment.py

cirq-core/cirq/experiments/xeb_fitting.py

maffoo · 2022-04-22T20:02:07Z

cirq-core/cirq/transformers/eject_phased_paulis_test.py

@@ -38,7 +38,11 @@ def assert_optimizes(
    if compare_unitaries:
        if cirq.is_parameterized(circuit):
            for a in (0, 0.1, 0.5, -1.0, np.pi, np.pi / 2):
-                params = {'x': a, 'y': a / 2, 'z': -2 * a}
+                params: Dict[Union[str, sympy.Expr], Union[float, sympy.Expr]] = {


nit: Use cirq.ParamDictType?

cirq-core/cirq/work/observable_settings.py

maffoo · 2022-04-22T20:07:16Z

cirq-core/cirq/work/sampler.py

@@ -29,6 +29,7 @@

 if TYPE_CHECKING:
    import cirq
+    import sympy


Is sympy still needed? Looks like it's not used below.

maffoo · 2022-04-22T20:11:35Z

cirq-google/cirq_google/serialization/circuit_serializer.py

@@ -434,54 +434,72 @@ def _deserialize_gate_op(
                exponent=arg_func_langs.float_arg_from_proto(
                    operation_proto.xpowgate.exponent,
                    arg_function_language=arg_function_language,
-                    required_arg_name=None,
+                    required_arg_name='exponent',


What's the point of this change? Seems like this will change functionality by requiring exponent to be set.

I think I did this for better error messages, but I just reverted it since it may have unintended effects.

maffoo · 2022-04-22T20:15:05Z

cirq-rigetti/cirq_rigetti/circuit_sweep_executors.py

@@ -57,7 +61,11 @@ def _execute_and_read_result(
        memory_map = {}

    for region_name, values in memory_map.items():
-        executable.write_memory(region_name=region_name, value=values)
+        print(region_name)


remove print statement

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

…into sympy_v10_part1

dstrain115 · 2022-04-27T01:10:30Z

Ok, everyone, please review! Your prompt attention would be appreciated, since this is quite challenging to keep up to date with merge conflicts.

mrwojtek · 2022-04-27T08:08:07Z

I'm fine with the changes under cirq_google.calibration

…into sympy_v10_part1

* Fix sympy 1.10 related mypy typing issues. - Fixes most of the typing issues related to sympy 1.10 - Most were caused by either: - Use of sympy.Basic which is too general (we probably want sympy.Expr) that did not have the right duck-typing - Not handling of sympy.Expr in various places that could theoretically have sympy expressions returned to them. - Fixed others with casts or other checks. - About ~10 out of 450 errors I just put type: ignore after struggling for a while. Co-authored-by: Matthew Neeley <mneeley@gmail.com>

dstrain115 added 4 commits April 15, 2022 15:56

Merge branch 'master' into sympy_v10_part1

a4adb9c

Formatting

fe7b7fb

Fix global phase stabilizer.

b9ad861

dstrain115 requested review from mrwojtek, a team, vtomole, cduck, dabacon, ColemanCollins, nakardo, gmauricio, wcourtney and verult as code owners April 15, 2022 23:21

CirqBot added the size: L 250< lines changed <1000 label Apr 15, 2022

Fix one instance of Basic

9459dd5

- Apparently boolean expressions in sympy are not sympy.Expr and are sympy.Basic. Sigh...

dabacon requested changes Apr 16, 2022

View reviewed changes

maffoo reviewed Apr 16, 2022

View reviewed changes

dstrain115 and others added 6 commits April 18, 2022 09:51

Fix typing with Durations.

32a43db

Update cirq-core/cirq/experiments/xeb_fitting.py

4bdd2ae

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

Update cirq-core/cirq/study/flatten_expressions.py

698e2fc

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

Fix ParamDictType

78dd73c

Merge branch 'sympy_v10_part1' of https://github.com/dstrain115/Cirq-1 …

4e82e29

…into sympy_v10_part1

Merge branch 'master' into sympy_v10_part1

b2a2bd2

dstrain115 requested a review from dabacon April 18, 2022 17:26

Fix type ignores.

2f5489c

dstrain115 requested a review from maffoo April 18, 2022 17:59

Formatting

294d2a3

MichaelBroughton assigned maffoo Apr 21, 2022

maffoo reviewed Apr 22, 2022

View reviewed changes

dstrain115 and others added 18 commits April 25, 2022 12:55

Update cirq-core/cirq/experiments/t2_decay_experiment.py

e46887c

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

Update cirq-core/cirq/interop/quirk/cells/single_qubit_rotation_cells.py

0e9a21f

Co-authored-by: Matthew Neeley <mneeley@gmail.com>

Remove sympy

0753281

Fix ParamDictType

576fe4f

Move exponent.

25c759f

Merge branch 'sympy_v10_part1' of https://github.com/dstrain115/Cirq-1 …

d8c19e1

…into sympy_v10_part1

Remove print.

6c49b35

Adjust dense pauli strings.

3082896

Apply formatting

9479a11

Merge branch 'master' into sympy_v10_part1

5e96e41

Fix TParamValComplex

7a167e1

Fix pylint issues.

d62ded1

Merge branch 'master' into sympy_v10_part1

bcae1f1

Adjust typing for complex values.

09629b2

pylint

44a028a

PArtial coverage fix

934a7fa

Finish coverage

0b55ee6

Merge branch 'master' into sympy_v10_part1

3f8d123

dstrain115 requested a review from maffoo April 27, 2022 01:09

dabacon approved these changes Apr 27, 2022

View reviewed changes

dstrain115 added 6 commits April 27, 2022 09:07

Merge branch 'master' into sympy_v10_part1

92cbd91

REfix act on stabilizer states typing.

5896388

Merge branch 'sympy_v10_part1' of https://github.com/dstrain115/Cirq-1 …

9f1579d

…into sympy_v10_part1

Add test for NotImplemented

d4a427b

Fix coverage.

0d13ced

Fix stabiilzer code coverage again.

a668ec1

dstrain115 merged commit e00767a into quantumlib:master Apr 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sympy 1.10 related mypy typing issues #5263

Fix sympy 1.10 related mypy typing issues #5263

dstrain115 commented Apr 15, 2022

dabacon Apr 16, 2022

maffoo Apr 16, 2022

dstrain115 Apr 18, 2022

dstrain115 Apr 18, 2022

dabacon Apr 16, 2022

dstrain115 Apr 18, 2022

dabacon Apr 16, 2022

dstrain115 Apr 18, 2022

maffoo left a comment

maffoo Apr 16, 2022

maffoo left a comment

maffoo Apr 22, 2022

dstrain115 Apr 25, 2022

maffoo Apr 22, 2022

dstrain115 Apr 25, 2022

dstrain115 Apr 25, 2022

maffoo Apr 22, 2022

dstrain115 Apr 25, 2022

dstrain115 Apr 25, 2022

maffoo Apr 22, 2022

dstrain115 Apr 25, 2022

maffoo Apr 22, 2022

dstrain115 Apr 25, 2022

dstrain115 commented Apr 27, 2022

mrwojtek commented Apr 27, 2022

		min_delay_nanos = min_delay_dur.total_nanos()
		max_delay_nanos = max_delay_dur.total_nanos()

Fix sympy 1.10 related mypy typing issues #5263

Fix sympy 1.10 related mypy typing issues #5263

Conversation

dstrain115 commented Apr 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maffoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maffoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstrain115 commented Apr 27, 2022

mrwojtek commented Apr 27, 2022