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

Speed up parameter resolution by checking if val is parameterized and caching the boolean #6023

Merged
merged 4 commits into from Mar 2, 2023

Conversation

tanujkhattar
Copy link
Collaborator

Fixes #6001

cc @maffoo @zchen088 PTAL!

Copy link
Collaborator

@zchen088 zchen088 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -35,16 +35,16 @@ def __init__(self, var):
self.parameter = var

def _is_parameterized_(self) -> bool:
return self.parameter == 0
return self.parameter != 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in the existing test, the _is_paramaterized_ returns True when the class is actually not paramaterized (i.e. self.parameter is 0 and there's nothing to resolve).

As a result cirq.is_parameterized(val) returns False when val is actually parameterized and therefore the output of cirq.resolve_parameters(val) changes.

So this change fixes the test to make the behavior of _is_paramaterized_ and resolve_parameters consistent.

cirq-core/cirq/ops/pauli_sum_exponential.py Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Outdated Show resolved Hide resolved
cirq-core/cirq/protocols/resolve_parameters.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed nits @maffoo

PTAL

@@ -35,16 +35,16 @@ def __init__(self, var):
self.parameter = var

def _is_parameterized_(self) -> bool:
return self.parameter == 0
return self.parameter != 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in the existing test, the _is_paramaterized_ returns True when the class is actually not paramaterized (i.e. self.parameter is 0 and there's nothing to resolve).

As a result cirq.is_parameterized(val) returns False when val is actually parameterized and therefore the output of cirq.resolve_parameters(val) changes.

So this change fixes the test to make the behavior of _is_paramaterized_ and resolve_parameters consistent.

cirq-core/cirq/protocols/resolve_parameters.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/raw_types.py Outdated Show resolved Hide resolved
cirq-core/cirq/ops/pauli_sum_exponential.py Show resolved Hide resolved
Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, then LGTM.

cirq-core/cirq/protocols/resolve_parameters.py Outdated Show resolved Hide resolved
@CirqBot CirqBot added the size: M 50< lines changed <250 label Mar 2, 2023
@tanujkhattar tanujkhattar merged commit 18991b5 into quantumlib:master Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance size: M 50< lines changed <250
Projects
None yet
4 participants