-
Notifications
You must be signed in to change notification settings - Fork 1k
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 Transformer API Interface and @cirq.transformer
decorator
#4797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. One high level question is do we need an interface for the statslogger or could we just make it concrete and put the implementation for register_initial and final inside that class ? I'm having trouble seeing what major differences each subclass might have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the comments.
For the logger, each subclass can chose to process, store and plot different information for the registered initial/final circuits (and their delta).
@maffoo @dstrain115 I've resolved most of the comments. The two big pending questions are:
|
@MichaelBroughton @dstrain115 @maffoo I've now also added the implementation of
One TODO left is to add support for writing the logs to a file instead of printing to stdout when Please take another look and give feedback. |
There was a problem hiding this 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 after some nits. Quick question: Do we expect to need a lot of class based transformers with __call__
defined ? Would we be able to keep things simple and just make them all standalone functions ?
…rcuit by default. Address comments
@MichaelBroughton @maffoo I've made all changes as per the feedback. The major change now is the change in transformer API that I described in #4797 (comment)
@MichaelBroughton The major problem in keeping transformers as standalone functions right now is the fact that we don't support additional arguments except circuit and context due to limitations in mypy. It should be possible to add support for additional args in the decorator using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for simplifying the types here; it is much easier to understand what is going on. I do still have a concern about using cirq.Circuit
by default, especially since the implementation is copying the circuit on each transformer invocation, despite the requirement that transformers not mutate their inputs. I'd suggest using AbstractCircuit
which should remove the need for separate TRANSFORMER
and TRANSFORMER_TO_DECORATE
types. But I think this is very close.
@dstrain115 Addressed all your comments. @maffoo Incorporated your suggestions, as per our offline discussions, for further simplifying the mypy types. A transformer is now just TRANSFORMER = Callable[['cirq.AbstractCircuit', TransformerContext], 'cirq.AbstractCircuit'] and the decorator implementation ensures that we don't modify the input type of a callable to preserve stricter return types wherever possible. There is still a small glitch that the transformer type does not enforce any constraint on the argument names. This is only a problem in a small corner case where @cirq.transformer
def f(c: cirq.AbstractCircuit, t: cirq.TransformerContext) -> cirq.Circuit:
pass
f(circuit, context) # This works fine because positional arguments are used for invocation.
f(c = circuit, t = context) # This will fail because we are explicitly using keyword arguments. To fix this, we'll have to change the transformer type to a protocol instead of a callable so we can enforce constraints on the argument names as well. However, I'd suggest let's do that in a separate PR as it's a small corner case and checking in this PR will help unblock all migration of all existing transformers. WDYT ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the internals of the trasformer
decorator can be cleaned up a bit, but that doesn't affect the exported API, so we can address that in a follow-up. LGTM!
Thanks @maffoo! Merging this now. |
Also cleans up some of the internals of the transformer decorator and simplifies the types. Follow-up to #4797
…tumlib#4797) Defines `TRANSFORMER_TYPE` and Implements the `@transformer` decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-api All existing transformers will be rewritten to follow the new API once this is checked-in. Implementation of `TransformerStatsLoggerBase` will follow in a separate PR. Part of quantumlib#4483 cc @maffoo PTAL at all the mypy magic.
* 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 #4797 * Fix Protocol import for 3.7 * Fixes from review * Add type annotations in transformer implementation Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
…tumlib#4797) Defines `TRANSFORMER_TYPE` and Implements the `@transformer` decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-api All existing transformers will be rewritten to follow the new API once this is checked-in. Implementation of `TransformerStatsLoggerBase` will follow in a separate PR. Part of quantumlib#4483 cc @maffoo PTAL at all the mypy magic.
…#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 <tanujkhattar@google.com>
…tumlib#4797) Defines `TRANSFORMER_TYPE` and Implements the `@transformer` decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-api All existing transformers will be rewritten to follow the new API once this is checked-in. Implementation of `TransformerStatsLoggerBase` will follow in a separate PR. Part of quantumlib#4483 cc @maffoo PTAL at all the mypy magic.
…#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 <tanujkhattar@google.com>
Defines
TRANSFORMER_TYPE
and Implements the@transformer
decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-apiAll existing transformers will be rewritten to follow the new API once this is checked-in.
Implementation of
TransformerStatsLoggerBase
will follow in a separate PR.Part of #4483
cc @maffoo PTAL at all the mypy magic.