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

GateFamily: add tags to str representation #5312

Merged
merged 5 commits into from
May 5, 2022

Conversation

verult
Copy link
Collaborator

@verult verult commented Apr 29, 2022

@verult verult requested review from a team, vtomole and cduck as code owners April 29, 2022 19:53
@CirqBot CirqBot added the size: S 10< lines changed <50 label Apr 30, 2022
Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

Add tests for this. Otherwise, looks fine to me.

@verult
Copy link
Collaborator Author

verult commented May 2, 2022

The default description change broken the JSON serialization test because the old description was captured in GateFamily.json for examples with tags set, and the repr of the deserialized object doesn't match what's in GateFamily.repr because of these lines:

if self.name != self._default_name() or self.description != self._default_description():
name_and_description = f'name="{self.name}", description="{self.description}", '

The old description no longer matches the new default description, so the repr contains a non-empty description, creating a conflict.

My proposed fix is to leave name and description empty in the JSON if they match the default, just like what we do for repr. IMO repr and json serialization logic should be similar ideally, and if the default name and description are used, there's no value in storing the full name and description anyway. As part of making this change, I also modified the existing objects in GateFamily.json and created inward files to make sure the original objects still deserialize properly (albeit to different objects from before).

@tanujkhattar tanujkhattar self-assigned this May 2, 2022
@verult
Copy link
Collaborator Author

verult commented May 3, 2022

Oof, to fix the presubmit failure, there's a bunch of GateFamilies in GridDeviceMetadata.json that also would need an _inward file. @dstrain115 @tanujkhattar I'll make that fix after you confirm the description serialization approach looks good.

@verult
Copy link
Collaborator Author

verult commented May 4, 2022

Discussed with @tanujkhattar offline. Opted to go with the solution which causes the least breakages to unblock the PR, which is to keep the logic the same and fix the description in GateFamily.json instead.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Looks great!

@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 4, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 5, 2022
@CirqBot CirqBot merged commit a49d8cc into quantumlib:master May 5, 2022
@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 May 5, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants