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

Fix @cirq.transformer decorator to work with default value of context, if provided. #4879

Closed
tanujkhattar opened this issue Jan 24, 2022 · 4 comments · Fixed by #4890
Closed
Assignees
Labels
area/transformers kind/task A task that's part of a larger effort

Comments

@tanujkhattar
Copy link
Collaborator

tanujkhattar commented Jan 24, 2022

Summarize the task
If default values are provided for any of the position arguments -- circuit or context, the @cirq.transformer decorator currently ignores them and the returned decorated method expects both arguments to be explicitly passed to the method.

This should be fixed so that the following works:

Acceptance criteria - when is the task considered done?

@cirq.transformer
def custom_transformer(
    circuit: cirq.Circuit, context: cirq.TransformerContext = cirq.TransformerContext()
) -> cirq.Circuit:
    pass

custom_transformer(cirq.Circuit()) # This should work. 

Related issues: #4483

cc @maffoo

@tanujkhattar tanujkhattar added kind/task A task that's part of a larger effort area/transformers labels Jan 24, 2022
@tanujkhattar tanujkhattar changed the title Fix @cirq.transformer decorator to work with "default" value of context if provided. Fix @cirq.transformer decorator to work with default value of context, if provided. Jan 24, 2022
@maffoo
Copy link
Contributor

maffoo commented Jan 24, 2022

I'd think we want the circuit arg to be required, and I think we should probably treat it like a positional-only argument (although language support for that in function signatures is only available in python 3.8+). For the context parameter, on the other hand, is there ever a reason why you would not want to allow a default parameter? I think having that universal default would be fairly straightforward in the decorator, having an optional default might be a bit more involved. Also, we might consider making context be a keyword-only argument.

@tanujkhattar
Copy link
Collaborator Author

For the context parameter, on the other hand, is there ever a reason why you would not want to allow a default parameter? I think having that universal default would be fairly straightforward in the decorator

One problem with making TransformerContext() a universal default in the decorator is that custom defaults will get over-written by the universal default defined in the decorator. i.e.

@cirq.transformer
def custom_transformer(circuit, context = TransformerContext(ignore_tags=('custom tag',))): 
# The custom default will get overwritten by `TransformerContext()` defined in decorator.
    pass

This can be a surprising behavior for users. The fix for this would probably also work for the optional default use case.

@maffoo
Copy link
Contributor

maffoo commented Jan 24, 2022

We can modify the decorator to inspect the signature of the decorated function and copy the default, if any. Something like:

        @functools.wraps(method)
        def method_with_logging(
            self, circuit: 'cirq.AbstractCircuit', *, context: TransformerContext = _default_context(method)
        ) -> 'cirq.AbstractCircuit':
            ...

def _default_context(callable):
    sig = inspect.signature(callable)
    default_context = sig.parameters["context"].default
    if default_context == inspect.Parameter.empty:
        default_context = TransformerContext()
    return default_context

@tanujkhattar
Copy link
Collaborator Author

I don't think we automatically assign a default TransformerContext() in decorator if it's not explicitly specified in the function definition. The reason is that in the decorator signature, we promise to preserve the input type. But, if we implicitly add a default value to context, the input type would not be preserved.

For example:

@cirq.transformer
def transformer_with_no_default(
    circuit: cirq.AbstractCircuit, context: cirq.TransformerContext
) -> cirq.Circuit:
    pass

transformer_with_no_default(circuit) # mypy will result in error because decorator preserves the original function type, which is one with no defaults. 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/transformers kind/task A task that's part of a larger effort
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants