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

Improvement in performance of resolve_parameters #6128

Merged
merged 2 commits into from Jun 11, 2023

Conversation

skushnir123
Copy link
Contributor

@skushnir123 skushnir123 commented Jun 9, 2023

This PR is in response to #6091 where it was brought up that cirq.resolve_parameters is very slow. This one-line PR speeds up the performance of the problem mentioned in the issue by a factor of ~6x (at least on my computer) and is comparable with qiskit's bind_parameters. The issue I found was that Sympy's subs function was being run on Floats which didn't do anything but took up the majority of the running time.

Before my change:

Elapsed cirq.resolve_parameters: 0.00469
Elapsed construct from scratch: 0.00033

After my change:

Elapsed cirq.resolve_parameters: 0.00079
Elapsed construct from scratch: 0.00027

Note: difference of ~6x

@skushnir123 skushnir123 requested review from a team, vtomole and cduck as code owners June 9, 2023 02:11
@CirqBot CirqBot added the Size: XS <10 lines changed label Jun 9, 2023
@tanujkhattar tanujkhattar self-assigned this Jun 9, 2023
@tanujkhattar
Copy link
Collaborator

Can you post results from running the before/after benchmarks from the original issue and also from https://github.com/quantumlib/Cirq/blob/master/benchmarks/parameter_resolution.py ?

@tanujkhattar
Copy link
Collaborator

@skushnir123
Copy link
Contributor Author

Can you also run the asv benchmark https://github.com/quantumlib/Cirq/blob/master/benchmarks/parameter_resolution.py ?

Yes, I will. I'm a bit busy today and tomorrow but will do on sunday.

@skushnir123 skushnir123 closed this Jun 9, 2023
@tanujkhattar
Copy link
Collaborator

@skushnir123 We can keep the PR open?

@tanujkhattar tanujkhattar reopened this Jun 9, 2023
@tanujkhattar
Copy link
Collaborator

I ran the benchmarks and it mostly looks good. I also think the parameter resolution benchmark should be probably be rewritten because there's a lot of time that goes in Circuit construction. Either way, this PR is good to merge. Thanks for the fix @skushnir123 !

       before           after         ratio
     [24abfa1d]       [abf0f389]
     <master>         <slow-resolve_parameters>
-        129±10ms          112±4ms     0.87  parameter_resolution.RabiCalibration.time_parameter_resolution(200, 20) [tanujkhattar-macbookpro2.roam.corp.google.com/virtualenv-py3.10-PYTHONOPTIMIZE-O]
-        508±30ms         440±20ms     0.87  parameter_resolution.RabiCalibration.time_parameter_resolution(150, 100) [tanujkhattar-macbookpro2.roam.corp.google.com/virtualenv-py3.10-PYTHONOPTIMIZE-O]
-        551±60ms         422±10ms     0.77  parameter_resolution.RabiCalibration.time_parameter_resolution(200, 80) [tanujkhattar-macbookpro2.roam.corp.google.com/virtualenv-py3.10-PYTHONOPTIMIZE-O]
-       504±100ms         312±20ms     0.62  parameter_resolution.RabiCalibration.time_parameter_resolution(200, 60) [tanujkhattar-macbookpro2.roam.corp.google.com/virtualenv-py3.10-PYTHONOPTIMIZE-O]

@tanujkhattar tanujkhattar enabled auto-merge (squash) June 11, 2023 00:53
@tanujkhattar tanujkhattar merged commit c08649f into quantumlib:master Jun 11, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: XS <10 lines changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants