Skip to content

Re-alias TrialResult to Result in json. #4319

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

Merged
merged 6 commits into from
Jul 15, 2021

Conversation

MichaelBroughton
Copy link
Collaborator

@MichaelBroughton MichaelBroughton commented Jul 14, 2021

Fixes #4318 .

Relatively small roll-back and re-aliasing of TrialResult. Json loading will now convert previously serialized TrialResult to a Result object. Confirmed on cirq==0.8.0 and cirq-dev

@MichaelBroughton MichaelBroughton requested review from cduck, vtomole and a team as code owners July 14, 2021 04:37
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Jul 14, 2021
Copy link
Contributor

@balopat balopat 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 nit: the json and repr files for backwards compatibility typically should be used with .json_inward and .repr_inward extensions to signal that these are only expected to be inputs, not outputs.

}
},
{
"cirq_type": "Result",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't look like it was saved with an old version of cirq

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No this file wasn't. The test seemed to do more explicit checking than doing just something like load_json and seeing that it worked. It would also look for the TrialResult class somewhere which we got rid of and don't want to put back (I don't think) I did check the functionality hit here by doing the following thing:

In Cirq==0.8.0:

circuit = cirq.Circuit(cirq.H(q), cirq.measure(q))
results = cirq.Simulator().run(circuit)
cirq.to_json(results)

Then in cirq master:

cirq.load_json(json_text=<text from previous snippet>)

Which would fail. From there I just added this entry into the resolver cache and it started working again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's what the _inward path is for as @balopat suggested

Copy link
Collaborator Author

@MichaelBroughton MichaelBroughton Jul 14, 2021

Choose a reason for hiding this comment

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

Ahhh ok! Went ahead and renamed to _inward and things are looking good now. Switched back to TrialResult too.

@@ -0,0 +1 @@
[cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[True, True, False, True, False], [False, True, True, False, False], [True, False, True, False, True]], dtype=np.bool)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[1, 1, 0, 1, 0], [0, 1, 1, 0, 0], [1, 0, 1, 0, 1]], dtype=np.uint8)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.int8)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.int16)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.int32)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.int64)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.uint8)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.uint16)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.uint32)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.uint64)}), cirq.Result(params=cirq.ParamResolver({sympy.Symbol('a'): 0.5}), measurements={'m': np.array([[1, 1, 0, 1, 0], [0, 1, 1, 0, 0], [1, 0, 1, 0, 1]], dtype=np.uint8), 'n': np.array([[0, 1, 2], [3, 4, 5], [6, 7, 8]], dtype=np.int64)})]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make too much sense to have actually, it's a bit of a lie, as it has cirq.Result as the type. We don't have cirq.TrialResult anymore, in my opinion repr based serialization is lost, unless we bring back cirq.TrialResult - which we don't want, that's why we deprecated it...

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this file and keep the json_inward and the type mapping for now.

Copy link
Collaborator Author

@MichaelBroughton MichaelBroughton Jul 15, 2021

Choose a reason for hiding this comment

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

The tests won't pass if I just delete the repr_inward. How do you want me to proceed ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc #4321 but this sortof makes sense: we use the repr file to generate the reference object to compare the json against. I.e. we need to create something to compare the old-format loaded-in value against. Probably not the best name. We can try to fix it as part of #4321 tooling improvements

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, sounds good, ¯_(ツ)_/¯

@balopat balopat self-assigned this Jul 15, 2021
@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 15, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 15, 2021
@CirqBot CirqBot merged commit c629c38 into quantumlib:master Jul 15, 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 Jul 15, 2021
erichulburd pushed a commit to erichulburd/Cirq that referenced this pull request Jul 15, 2021
Fixes quantumlib#4318  .

Relatively small roll-back and re-aliasing of `TrialResult`. Json loading will now convert previously serialized `TrialResult` to a `Result` object. Confirmed on `cirq==0.8.0` and `cirq-dev`
@mpharrigan
Copy link
Collaborator

mpharrigan commented Jul 22, 2021

Are there any plans to cherry pick this into an 0.11.1 release? Otherwise ReCirq is blocked to update.

MichaelBroughton added a commit to MichaelBroughton/Cirq that referenced this pull request Jul 26, 2021
Fixes quantumlib#4318  .

Relatively small roll-back and re-aliasing of `TrialResult`. Json loading will now convert previously serialized `TrialResult` to a `Result` object. Confirmed on `cirq==0.8.0` and `cirq-dev`
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Fixes quantumlib#4318  .

Relatively small roll-back and re-aliasing of `TrialResult`. Json loading will now convert previously serialized `TrialResult` to a `Result` object. Confirmed on `cirq==0.8.0` and `cirq-dev`
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Fixes quantumlib#4318  .

Relatively small roll-back and re-aliasing of `TrialResult`. Json loading will now convert previously serialized `TrialResult` to a `Result` object. Confirmed on `cirq==0.8.0` and `cirq-dev`
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.

JSON backwards compatibility has been flushed
4 participants