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

Support gzip serialization #3662

Merged
merged 10 commits into from
Feb 1, 2021
Merged

Conversation

95-martin-orion
Copy link
Collaborator

For comparison with #3655. Observed benchmark results, running locally:

nested: 7.016707049 s
size: 299304
flattened: 1.278540381 s
size: 20911

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jan 11, 2021
@95-martin-orion 95-martin-orion changed the title [DO NOT SUBMIT] Example gzip serialization w/benchmark. Support gzip serialization Jan 12, 2021
@95-martin-orion
Copy link
Collaborator Author

Benchmarks have been migrated to the dev_tools/profiling directory. Further tests suggest that the example described above may be an outlier with regards to gzip-serialized data size of heavily-nested circuits - with more gates and slightly less nesting, data size was comparable with (or better than) JSON-serialization.

@95-martin-orion 95-martin-orion marked this pull request as ready for review January 13, 2021 17:12
@95-martin-orion 95-martin-orion requested review from cduck, vtomole and a team as code owners January 13, 2021 17:12
@mpharrigan
Copy link
Collaborator

xref #3438

@95-martin-orion
Copy link
Collaborator Author

xref #3438

I'm sure I saw this issue at some point, but failed to review it prior to this PR. Would you like me to repackage this into the format discussed there? (i.e. to_json({...}, compress=['json','gzip','no',None]))

@mpharrigan
Copy link
Collaborator

Would you like me to repackage this

No, just wanted to make sure there was reference to the other discussion

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

a minor qualm and a major qualm. What should this function be named?

cirq/protocols/json_serialization.py Outdated Show resolved Hide resolved
cirq/protocols/json_serialization.py Outdated Show resolved Hide resolved
cirq/protocols/json_serialization.py Show resolved Hide resolved
dev_tools/profiling/benchmark_serializers.py Outdated Show resolved Hide resolved
@95-martin-orion 95-martin-orion added the Ready for Re-Review For when reviewers take their time. label Jan 29, 2021
Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

lgtm. sorry about the delay this must have gotten lost in my inbox

@google-cla
Copy link

google-cla bot commented Feb 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes Makes googlebot stop complaining. labels Feb 1, 2021
@95-martin-orion 95-martin-orion added automerge Tells CirqBot to sync and merge this PR. (If it's running.) cla: yes Makes googlebot stop complaining. and removed Ready for Re-Review For when reviewers take their time. cla: no labels Feb 1, 2021
@CirqBot
Copy link
Collaborator

CirqBot commented Feb 1, 2021

Automerge cancelled: A status check is failing.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 1, 2021
@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 1, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 1, 2021
@google-cla
Copy link

google-cla bot commented Feb 1, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@CirqBot
Copy link
Collaborator

CirqBot commented Feb 1, 2021

Automerge cancelled: A status check is failing.

@google-cla google-cla bot added cla: no and removed cla: yes Makes googlebot stop complaining. labels Feb 1, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 1, 2021
@95-martin-orion
Copy link
Collaborator Author

Ah, the CLA doesn't like the email address that cf706f4 is associated with. I'll rebase and force-push to appease it.

@google-cla google-cla bot added cla: yes Makes googlebot stop complaining. and removed cla: no labels Feb 1, 2021
@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 1, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 1, 2021
@CirqBot CirqBot merged commit 3c566d2 into quantumlib:master Feb 1, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants