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

Use a Protocol for TRANSFORMER to ensure common arg names #4871

Merged
merged 6 commits into from
Jan 24, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Jan 21, 2022

Also cleans up some of the internals of the transformer decorator by inlining the implementations and simplifies the types.

Follow-up to #4797

Also cleans up some of the internals of the transformer decorator and
simplifies the types.

Follow-up to #4797
@maffoo maffoo requested review from cduck, vtomole and a team as code owners January 21, 2022 23:15
@CirqBot CirqBot added the size: M 50< lines changed <250 label Jan 21, 2022
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.

LGTM % minor nits.

This looks great, but I have so many doubts regarding why this working!

Kudos for the amazing cleanup btw!

cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
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.

Thanks for the cleanup -- this was a nice strategy to preserve types only in the decorator definition and omit type annotations in the implementation.

@tanujkhattar tanujkhattar merged commit 693ceed into master Jan 24, 2022
@maffoo maffoo deleted the u/maffoo/transformer-types branch January 24, 2022 18:36
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…#4871)

* Use a Protocol for TRANSFORMER to ensure common arg names

Also cleans up some of the internals of the transformer decorator and
simplifies the types.

Follow-up to quantumlib#4797

* Fix Protocol import for 3.7

* Fixes from review

* Add type annotations in transformer implementation

Co-authored-by: Tanuj Khattar <[email protected]>
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…#4871)

* Use a Protocol for TRANSFORMER to ensure common arg names

Also cleans up some of the internals of the transformer decorator and
simplifies the types.

Follow-up to quantumlib#4797

* Fix Protocol import for 3.7

* Fixes from review

* Add type annotations in transformer implementation

Co-authored-by: Tanuj Khattar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants