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

Caching value_equality_values? #6272

Closed
zchen088 opened this issue Aug 30, 2023 · 3 comments · Fixed by #6275
Closed

Caching value_equality_values? #6272

zchen088 opened this issue Aug 30, 2023 · 3 comments · Fixed by #6275
Assignees
Labels
area/performance good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. no QC knowledge needed Want to contribute to Cirq, but don't know quantum computing? This issue is for you. 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 feature request related to a use case or problem? Please describe.

Are we "allowed" to cache the _value_equality_values? If so, is there a standard way to do this?

I'm in particular looking at PhasedXZGate:

def _value_equality_values_(self):
and computing the hash here is relatively expensive.

Describe the solution you'd like
I suppose decorating with @_compat.cached_method would be the easiest thing.

[optional] Describe alternatives/workarounds you've considered

[optional] Additional context (e.g. screenshots)

What is the urgency from your perspective for this issue? Is it blocking important work?

P1 - I need this no later than the next release (end of quarter)

@zchen088 zchen088 added the kind/feature-request Describes new functionality label Aug 30, 2023
@senecameeks senecameeks added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. no QC knowledge needed Want to contribute to Cirq, but don't know quantum computing? This issue is for you. and removed kind/feature-request Describes new functionality labels Aug 30, 2023
@maffoo
Copy link
Contributor

maffoo commented Aug 30, 2023

Yes, caching this method should be fine if the underlying is meant to be immutable, as is certainly the case for gates like PhasedXZGate. Decorating the method with @cached_method is the way to do this, as you note (we introduced the cached_method decorator for precisely this reason).

@maffoo
Copy link
Contributor

maffoo commented Aug 30, 2023

I will say that to fully take advantage of the performance benefit of caching, you'll want to reuse instances of PhasedXZGate, which might require some restructuring of the code that is using PhasedXZGate.

@tanujkhattar
Copy link
Collaborator

From cirq sync: We should actually just modify the @value.value_equality decorator implementation to

  • Add a cache to the _value_equality_values_ and _value_equality_approximate_values_
  • Add a cache to the auto generated _hash_ functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance good first issue This issue can be resolved by someone who is not familiar with the codebase. A good starting issue. no QC knowledge needed Want to contribute to Cirq, but don't know quantum computing? This issue is for you. 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.

4 participants