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

Add explicit JSON resolver function in contrib #2559

Merged
merged 19 commits into from
Dec 2, 2019
Merged

Add explicit JSON resolver function in contrib #2559

merged 19 commits into from
Dec 2, 2019

Conversation

KevinVillela
Copy link
Contributor

Fixes #2445

This PR adds an explicit function to return a JSON serializer containing Contrib classes. Right now, only QuantumVolumeResult is JSONable, but in the future other classes can be added here as well. Users who want to use the JSON family of functions will need to explicitly pass the contrib_class_resolver, as is done in the test.

Additionally, this pulls out a useful json_test function into the testing directory, for use elsewhere.

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Nov 15, 2019
import cirq


def assert_roundtrip(obj, text_should_be=None, resolvers=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is too vague; there is not enough context to know what cirq.testing.assert_roundtrip refers to.

Rename this to something more explicit like assert_json_roundtrip_works.

We may want to include this into the standard test battery for gates (assert_implements_consistent_protocols).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the name, thanks!

I tried adding this to assert_implements_consistent_protocols, but that function is called with a ton of test gates (e.g. GateUsingWorkspaceForApplyUnitary) :\

classes = [
QuantumVolumeResult,
]
print(QuantumVolumeResult.__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover debug line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh I fixed that and forgot to commit it :P Done.

@KevinVillela
Copy link
Contributor Author

Friendly ping :)

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.

this looks good to me


"""


Copy link
Collaborator

Choose a reason for hiding this comment

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

you could add

DEFAULT_RESOLVERS = [contrib_class_resolver] + protocols.json.DEFAULT_RESOLVERS

then

import cirq
import cirq.contrib
data = cirq.read_json(fn, resolvers=cirq.contrib.DEFAULT_RESOLVERS)

The above relies on python namespacing for disambiguation. You could make the name more specific: DEFAULT_CONTRIB_RESOLVERS if you want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done and used in the test.

@mpharrigan
Copy link
Collaborator

@Strilanc you good?

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 2, 2019
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 2, 2019
@CirqBot
Copy link
Collaborator

CirqBot commented Dec 2, 2019

Automerge cancelled: Need a fresh 🍪.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 2, 2019
@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 2, 2019
@CirqBot CirqBot merged commit 42f9a37 into quantumlib:master Dec 2, 2019
@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 Dec 2, 2019
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.

(re-)register deserializer for QuantumVolumeResult
5 participants