Skip to content

Conversation

dstrain115
Copy link
Collaborator

  • Adds support for (potentially nested) tuples of mismatched types
  • Adds support for serializing ndarrays
  • Adds support for serializing complex numbers

- Adds support for (potentially nested) tuples of mismatched types
- Adds support for serializing ndarrays
- Adds support for serializing complex numbers
@dstrain115 dstrain115 requested review from a team, verult, vtomole and wcourtney as code owners April 3, 2025 22:33
Copy link

codecov bot commented Apr 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (02941bf) to head (d77b78a).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #7226    +/-   ##
========================================
  Coverage   98.65%   98.65%            
========================================
  Files        1106     1106            
  Lines       95869    95969   +100     
========================================
+ Hits        94581    94681   +100     
  Misses       1288     1288            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pavoljuhas pavoljuhas added the priority/before-1.5 Finish before the Cirq 1.5 release label Apr 4, 2025
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

Please enable handling of empty numpy arrays. Also consider storing the list or tuple type in the proto so they can be restored as the same type (can be a follow-up PR).

Otherwise LGTM.

Comment on lines 456 to 457
double real_value = 1;
double imaginary_value = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use real and imag for parity with builtin complex and numpy numeric types?

Comment on lines 188 to 189
match value.dtype.name:
case 'float64':
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can match value.dtype rather than the name. This would protect against typo in string or a rename of data 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.

Done.

Comment on lines 371 to 373
return (
arg_value.complex_value.real_value
+ 1j * arg_value.complex_value.imaginary_value
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
return (
arg_value.complex_value.real_value
+ 1j * arg_value.complex_value.imaginary_value
return complex(
arg_value.complex_value.real_value,
arg_value.complex_value.imaginary_value

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 144 to 145
elif isinstance(value, (list, tuple, np.ndarray)):
if len(value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

On a round trip this converts (i) uniform-type lists or tuples to lists and (ii) non-uniform lists and tuples to tuples. It also discards empty lists, tuples and arrays, which return as None from round trip.

We can move the array conversion out of the if len(value) block as _ndarray_to_proto and _ndarray_from_proto seem to work with empty arrays.

As for tuples and lists, we could add a sequence_type enum to the ArgValue message which could be (UNSPECIFIED, LIST, TUPLE) and would let us reconstruct either list or tuple from a round trip. It would also allow us to express either an empty list or an empty tuple, in such case ArgValue would be empty except of a (new) sequence_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.

Good idea! I went ahead and did that, and also added set support while in there.

@dstrain115 dstrain115 requested a review from pavoljuhas April 7, 2025 13:05
@dstrain115 dstrain115 requested a review from senecameeks April 7, 2025 17:12
Copy link
Collaborator

@pavoljuhas pavoljuhas left a comment

Choose a reason for hiding this comment

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

LGTM with a tiny suggestion.

@dstrain115 dstrain115 enabled auto-merge April 7, 2025 20:37
@dstrain115 dstrain115 added this pull request to the merge queue Apr 7, 2025
Merged via the queue into quantumlib:main with commit d39112e Apr 7, 2025
38 checks passed
@dstrain115 dstrain115 deleted the u/dstrain/add_tuples_to_proto branch April 7, 2025 21:07
BichengYing pushed a commit to BichengYing/Cirq that referenced this pull request Jun 20, 2025
…umlib#7226)

* Add tuples, ndarrays, and complex numbers to cirq_google proto

- Adds support for (potentially nested) tuples of mismatched types
- Adds support for serializing ndarrays
- Adds support for serializing complex numbers

* my o my, it's mypy

* Fix all the review comments, add set support.

* Fix invert mask, which assumes list, so it can accept tuples.

* Update cirq-google/cirq_google/serialization/arg_func_langs.py

Co-authored-by: Pavol Juhas <[email protected]>

* Fix coverage.

---------

Co-authored-by: Pavol Juhas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority/before-1.5 Finish before the Cirq 1.5 release

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants