Add NumPy number types to TParamVal - #8236
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8236 +/- ##
========================================
Coverage 99.59% 99.59%
========================================
Files 1125 1126 +1
Lines 103250 103679 +429
========================================
+ Hits 102829 103258 +429
Misses 421 421 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mhucka
left a comment
There was a problem hiding this comment.
Thank you for this work.
To go along with these changes, an additional overload for canonicalize_half_turns for np.number (or TypeVar bounded by np.number) is needed in file cirq/value/angle.py.
The test cases also should be expanded to exercise both floating-point and integer NumPy scalar subtypes (e.g., np.float64, np.double, np.int64, np.short) to verify that arithmetic operations (e.g., modulo operations, and range wrapping into (-1, +1]) preserve type and value invariants as expected across NumPy numeric types.
Finally, Dax Fohl noted in the last comment on issuue 5758 that a change of this nature would have backward compatibility implications. Can you add test cases (e.g., for the case mentioned at the end of the first paragraph in that comment) to verify that backards compatibility is maintained?
|
Discussed during Cirq Cynq 2026-08-19: consensus is that this change may indeed work, but this definitely needs many more test cases than what is in the PR currently. In particular, we need to check that the parameter resolver works correctly. (See A possible approach is to look at the tests already in param resolver, then extend the relevant tests with numpy values, and make sure everything still works. |
987c1a3 to
b54346b
Compare
|
Rebased onto main and fixed format in b54346b. |
b54346b to
d4780eb
Compare
|
Rebased onto main in d4780eb. |
|
Extended the existing 546 passed |
|
added a parametrized test documenting the isinstance(exponent, float) split: true for np.float64/np.double, false for np.float32 and the int dtypes, same before and after this pr since widening TParamVal is annotation-only. also locks in that resolve_parameters normalizes every dtype to plain float regardless. |
mhucka
left a comment
There was a problem hiding this comment.
Thank you for this effort!
I have some initial review comments.
mhucka
left a comment
There was a problem hiding this comment.
Thanks for the updates!
Could some additional tests be added for the following too?
-
Verify roundtrip equality for gates and circuits parameterized with various NumPy scalar dtypes. (E.g.,
cirq.to_json()must serialize gates with NumPy scalar parameters without throwing TypeError, andcirq.read_json()needs to reconstruct them with standard Python numeric types.) -
Check that
cirq.Gate.with_probability(p)and channel classes (such ascirq.DepolarizingChannel,cirq.BitFlipChannel, etc.) acceptTParamValor callvalidate_probability. -
Check that
canonicalize_half_turnshandles unsigned integer types (such as half_turns -= 2 when values exceed 1) without overflow or type errors. Maybe add test cases that use types np.uint8, np.uint16, np.uint32, np.uint64. -
Check what happens with 0-dimensional np.ndarrays.
-
Check floating-point value edge cases: signed zero, NaN, Inf. For example, checking that
canonicalize_half_turns(np.float64(-0.0))is handled without creating negative zero mismatches in gate equality, checking thatcirq.is_parameterized(cirq.XPowGate(exponent=np.nan))returnsFalse, and so on.
|
@pavoljuhas and/or @daxfohl, would you be able to take a look at this PR too? |
1717243 to
5066cf8
Compare
TParamVal was float | sympy.Expr, but parameter values (e.g. EigenGate exponents) already accept NumPy scalars like np.double at runtime. Mirror TParamValComplex by including np.number so static type checkers agree with the established runtime behavior. Fixes quantumlib#5758 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Pytest Ubuntu (3.14) failed on stream_manager_test cancel race; Mac/Win 3.14 and other Ubuntu versions passed. PR only touches TParamVal.
Add an np.number TypeVar overload for canonicalize_half_turns and cover float/integer NumPy scalars, ParamResolver, and the quantumlib#5758 isinstance case. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Cover np.float32/np.int32 through canonicalize_half_turns wrapping into (-1, +1], TParamVal, ParamResolver, and the quantumlib#5758 np.double isinstance case. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Add NumPy values to the existing resolve_parameters, is_parameterized, parameter_names, and recursive resolve tests. Cover ParamResolver formulas plus rx/ry/rz, FSim, PhasedXZ, GlobalPhase, Wait, CCZ/CCX, and controlled XPow. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
Backward compat only holds for np.float64/np.double, whose NumPy scalar type subclasses Python float; np.float32 and the integer dtypes do not. This was already true before TParamVal was widened (widening the alias is annotation-only), but was implicit. Make it an explicit, parametrized test so the boundary is documented rather than assumed. Also covers that resolve_parameters normalizes every dtype to a plain float on resolution, regardless of the pre-resolution isinstance result.
Serialize gates and circuits parameterized with NumPy scalar dtypes through cirq.to_json/read_json as Python numbers. Accept those scalars on Gate.with_probability and channel classes that call validate_probability. Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>
21b6491 to
df77245
Compare
Move TParamVal membership checks into type_alias_test.py, document that the canonicalize_half_turns TypeVar excludes complex np.number types, and wrap 0-d arrays through (-1, +1]. Co-authored-by: Sankalp Thakur <sankalpsthakur@users.noreply.github.com>
|
Addressed the remaining review notes on this tip (
Ready for another look when you have time. |
|
Should be fine. It's just a type change so it's not actually breaking any runtime behavior. The two backwards incompatibilities are
So, formally if we're doing tight type controls, this is breaking. But I can't think of any scenarios where it would cause any real-world problems. LGTM. |
|
The remaining macOS failure is |
Fixes #5758.
Summary
TParamValwas defined asfloat | sympy.Expr, while parameter values such asEigenGateexponents already accept NumPy scalars (e.g.np.double,np.short) at runtime. This change addsnp.numberso the type alias matches that behavior, consistent withTParamValComplexwhich already includesnp.number.Notes
TParamValInput/ conversion layer).Scalarfromlinear_dict.py(orthogonal follow-up if desired).# TODO(#5758)marker next to existing serializer tests that already covernp.double/np.shortexponents.np.numberTypeVar overload oncanonicalize_half_turns.canonicalize_half_turnsand ParamResolver tests.Test plan
pytest cirq-core/cirq/value/angle_test.py cirq-core/cirq/protocols/resolve_parameters_test.py cirq-core/cirq/ops/eigen_gate_test.py cirq-core/cirq/study/resolver_test.pycirq-googleserializer cases already exercise NumPy exponents end-to-endAI/LLM disclosure