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.

Sign up for GitHub

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

Jump to bottom

Add name to circuit serializers #4422

Merged
merged 10 commits into from
Aug 19, 2021
Merged

Add name to circuit serializers #4422

merged 10 commits into from
Aug 19, 2021

Conversation

dstrain115
Copy link
Collaborator

  • name is needed since it is referenced by the protos (gate_set_name)
  • This is in both CircuitSerializer and SerializableGateSet but also
    needs to be in the interface to be used generally.
  • Added name() property and kept gate_set_name as a property for
    backwards compatibility.

@dstrain115
Add name to circuit serializers
9d0ec77 
- name is needed since it is referenced by the protos (gate_set_name)
- This is in both CircuitSerializer and SerializableGateSet but also
needs to be in the interface to be used generally.
- Added name() property and kept gate_set_name as a property for
backwards compatibility.
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Aug 12, 2021
@CirqBot CirqBot added the size: M 50< lines changed <250 label Aug 12, 2021
MichaelBroughton
MichaelBroughton reviewed Aug 14, 2021
View reviewed changes
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

Do you think it would it be worth deprecating the gate_set_name field as well ?

@MichaelBroughton MichaelBroughton self-assigned this Aug 14, 2021
@dstrain115
Copy link
Collaborator Author

Do you think it would it be worth deprecating the gate_set_name field as well ?

Maybe, but I couldn't figure out how. I guess you cannot decorate property classes, so it wouldn't let me add a deprecation decorator onto it. Since I am moving everything to using the Serializer interface which doesn't have the old 'gate_set_name' property, I didn't worry about it much. If you have a good way to add a deprecation to a @property though, I can go ahead and do it.

@MichaelBroughton
Copy link
Collaborator

Did you try something like: #4432 ?

@dstrain115
Copy link
Collaborator Author

Maybe, but I couldn't figure out how. I guess you cannot decorate property classes, so it wouldn't let me add a deprecation decorator onto it. Since I am moving everything to using the Serializer interface which doesn't have the old 'gate_set_name' property, I didn't worry about it much. If you have a good way to add a deprecation to a @property though, I can go ahead and do it.

Mypy complains, but I just told it to ignore it.

MichaelBroughton
MichaelBroughton approved these changes Aug 19, 2021
View reviewed changes
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

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

CirqBot commented Aug 19, 2021

Automerge cancelled: A status check is failing.

@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 Aug 19, 2021
@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 19, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Aug 19, 2021
@CirqBot CirqBot merged commit d259a8c into quantumlib:master Aug 19, 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 Aug 19, 2021
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
@dstrain115 @rht
Add name to circuit serializers (quantumlib#4422)
a315f51 
- name is needed since it is referenced by the protos (gate_set_name)
- This is in both CircuitSerializer and SerializableGateSet but also
needs to be in the interface to be used generally.
- Added name() property and kept gate_set_name as a property for
backwards compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelBroughton MichaelBroughton MichaelBroughton approved these changes

@cduck cduck Awaiting requested review from cduck

@vtomole vtomole Awaiting requested review from vtomole

@wcourtney wcourtney Awaiting requested review from wcourtney

@viathor viathor Awaiting requested review from viathor

Assignees

@MichaelBroughton MichaelBroughton

Labels
cla: yes Makes googlebot stop complaining. size: M 50< lines changed <250
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dstrain115 @MichaelBroughton @CirqBot