Skip to content

Should _resolve_value pass None through? #4029

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

Closed
zchen088 opened this issue Apr 16, 2021 · 4 comments · Fixed by #5466
Closed

Should _resolve_value pass None through? #4029

zchen088 opened this issue Apr 16, 2021 · 4 comments · Fixed by #5466
Assignees
Labels
area/parameters parameter resolution, parameterized gates, operations kind/design-issue A conversation around design triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@zchen088
Copy link
Collaborator

Is your design idea/issue related to a use case or problem? Please describe.
In some cases, we'd like to be able to resolve a parameter to None to signal that the gate should use "default behavior". Previously, None was being used to signal recursion in the parameter resolution code, but this is no longer the case, so it seems like we should be able to do this.

Describe your design idea/issue
This would be relatively simple change. In _resolve_value:

def _resolve_value(val: Any) -> Any:
, we would simply check is None and pass that on through. What I'm not sure about is, are we relying on None not being pass through anywhere else in Cirq? For example, is there code which assumes that, if the value is None, don't resolve it?

@zchen088 zchen088 added the kind/design-issue A conversation around design label Apr 16, 2021
@maffoo
Copy link
Contributor

maffoo commented Apr 16, 2021

This seems reasonable to me. I think basically all "primitive" values should get passed through _resolve_value since they obviously can't meaningfully be resolved to anything else. Could you give a snippet showing the current behavior and how you'd like things to work?

@95-martin-orion
Copy link
Collaborator

What I'm not sure about is, are we relying on None not being pass through anywhere else in Cirq? For example, is there code which assumes that, if the value is None, don't resolve it?

I'm not aware of anything like that. My current expectation is that something like cirq.X ** phi, params={phi: None} would fail, but I don't think any Cirq code actually relies on this. However, the expectation of failure is much stronger for cirq.X ** phi, params={} (i.e. phi is unassigned) - that should continue to fail after this change.

If the requirement isn't specifically for None to provide default behavior, I might suggest using a stub object like _RecursionFlag instead.

@balopat balopat added the area/parameters parameter resolution, parameterized gates, operations label May 4, 2021
@verult verult added the triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on label Mar 23, 2022
@verult
Copy link
Collaborator

verult commented Mar 23, 2022

Marking as triage/accepted based on comments.

@dstrain115
Copy link
Collaborator

I will go ahead and take this on since there hasn't been movement on this in two months. Please let me know if anyone has already started on this.

dstrain115 added a commit to dstrain115/Cirq-1 that referenced this issue Jun 7, 2022
- pass through None as a parameter if set.

Fixes: quantumlib#4029
CirqBot pushed a commit that referenced this issue Jun 7, 2022
- pass through None as a parameter if set.

Technically a breaking change, since this used to silently drop parameters that were set as None. 

Fixes: #4029
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
- pass through None as a parameter if set.

Technically a breaking change, since this used to silently drop parameters that were set as None. 

Fixes: quantumlib#4029
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
- pass through None as a parameter if set.

Technically a breaking change, since this used to silently drop parameters that were set as None. 

Fixes: quantumlib#4029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/parameters parameter resolution, parameterized gates, operations kind/design-issue A conversation around design triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants