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

Optimize parameter resolution by avoiding redundant calls to _resolve_parameters_ if _is_parameterized_ is False #6001

Closed
tanujkhattar opened this issue Feb 8, 2023 · 3 comments · Fixed by #6023
Assignees
Labels
area/parameters parameter resolution, parameterized gates, operations area/performance kind/task for tracking progress on larger efforts no QC knowledge needed Want to contribute to Cirq, but don't know quantum computing? This issue is for you. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add

Comments

@tanujkhattar
Copy link
Collaborator

Summarize the task
cirq.resolve_parameters protocol delegates to the _resolve_parameters_ method of the value for which we need to resolve parameters of. However, this should happen only if the value is parameterized in the first place, to avoid doing redundant work trying to resolve parameters (by creating unnecessary copies for example ).

Acceptance criteria - when is the task considered done?
As part of making parameter resolution more performant, we should:

  1. Check that we should call value._resolve_parameters_() only if value._is_parameterized_() returns True.
  2. Cache the results of _is_parameterized_ for FrozenCircuit and Moment because they are immutable objects.
@tanujkhattar tanujkhattar added no QC knowledge needed Want to contribute to Cirq, but don't know quantum computing? This issue is for you. kind/task for tracking progress on larger efforts area/parameters parameter resolution, parameterized gates, operations area/performance labels Feb 8, 2023
@tanujkhattar tanujkhattar self-assigned this Feb 8, 2023
@maffoo
Copy link
Contributor

maffoo commented Feb 8, 2023

Gate and Operation should be immutable too. would be good to cache _is_parameterized_ for them as well. Might have to do this on subclass at a time for gates.

@pavoljuhas pavoljuhas added the triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add label Feb 10, 2023
@zchen088
Copy link
Collaborator

To be specific, I think we're talking about adding @_compat.cached_method to invocations of _is_parameterized_. If this sounds good, I could maybe try doing this (though it's been a while since I've had a Cirq dev environment spun up).

@tanujkhattar
Copy link
Collaborator Author

@zchen088 That's correct, we should be using @_compat.cached_method to make sure we include self in the cache.

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 area/performance kind/task for tracking progress on larger efforts no QC knowledge needed Want to contribute to Cirq, but don't know quantum computing? This issue is for you. triage/accepted there is consensus amongst maintainers that this is a real bug or a reasonable feature to add
Projects
None yet
4 participants