Skip to content

Revert "Set sympy.evaluate(False) on frequent code paths for phase tracking. (#6072)"#6386

Merged
mergify[bot] merged 3 commits intoQiskit:mainfrom
mtreinish:revert-sympy
May 12, 2021
Merged

Revert "Set sympy.evaluate(False) on frequent code paths for phase tracking. (#6072)"#6386
mergify[bot] merged 3 commits intoQiskit:mainfrom
mtreinish:revert-sympy

Conversation

@mtreinish
Copy link
Copy Markdown
Member

Summary

With recent adoption of symengine for the performance critical path
with ParameterExpressions the workaround for large expression evaluation
in #6072 is no longer needed or applicable, except in environments
without symengine. However, with symengine available using the sympy
evaluate context adds noticeable overhead, both to import sympy (which
wouldn't be used otherwise) and to load the sympy evalue context
wrapper. This commit removes these as we shouldn't compromise performance
in the performance path to try and speed up a path with degraded
performance because symengine isn't available (32bit platforms).

Details and comments

This reverts commit f436f5a.

…acking. (Qiskit#6072)"

With recent adoption of symengine for the performance critical path
with ParameterExpressions the workaround for large expression evaluation
in Qiskit#6072 is no longer needed or applicable, except in environments
without symengine. However, with symengine available using the sympy
evaluate context adds noticeable overhead, both to import sympy (which
wouldn't be used otherwise) and to load the sympy evalue context
wrapper. This commit removes these as we shouldn't compromise performance
in the performance path to try and speed up a path with degraded
performance because symengine isn't available (32bit platforms).

This reverts commit f436f5a.
@mtreinish mtreinish requested a review from kdk May 10, 2021 11:52
@mtreinish mtreinish requested a review from a team as a code owner May 10, 2021 11:52
@mtreinish
Copy link
Copy Markdown
Member Author

Running the example from the original issue that #6072 was fixing with this PR returned:

ExcitationPreserving(num_qubits=6, reps=3)
Construction time: 0.0006351470947265625 seconds
Number of instructions: 114
Transpilation time: 0.07029318809509277 seconds

ExcitationPreserving(num_qubits=12, reps=6)
Construction time: 0.0003178119659423828 seconds
Number of instructions: 876
Transpilation time: 0.3877732753753662 seconds

ExcitationPreserving(num_qubits=16, reps=8)
Construction time: 0.0007126331329345703 seconds
Number of instructions: 2064
Transpilation time: 1.1844496726989746 seconds

ExcitationPreserving(num_qubits=20, reps=10)
Construction time: 0.0016641616821289062 seconds
Number of instructions: 4020
Transpilation time: 2.9190211296081543 seconds

ExcitationPreserving(num_qubits=24, reps=12)
Construction time: 0.0025053024291992188 seconds
Number of instructions: 6936
Transpilation time: 6.337162733078003 seconds

so using symengine alleviates the slow sympy evaluation #6072 was working around.

@mtreinish mtreinish linked an issue May 11, 2021 that may be closed by this pull request
@kdk kdk added the automerge label May 11, 2021
@kdk
Copy link
Copy Markdown
Member

kdk commented May 11, 2021

Can #6187 likewise be reverted?

@mtreinish
Copy link
Copy Markdown
Member Author

Can #6187 likewise be reverted?

I would expect we probably could but I wouldn't expect much of a performance change from it. I feel like a dict lookup will still be faster than symengine, but not nearly as much, but either way we'lll have to benchmark it to be sure.

@mergify mergify Bot merged commit 9b78934 into Qiskit:main May 12, 2021
@mtreinish mtreinish deleted the revert-sympy branch May 12, 2021 14:29
@kdk
Copy link
Copy Markdown
Member

kdk commented May 24, 2021

Can #6187 likewise be reverted?

I would expect we probably could but I wouldn't expect much of a performance change from it. I feel like a dict lookup will still be faster than symengine, but not nearly as much, but either way we'lll have to benchmark it to be sure.

True, the benefit in that case would be mostly for code maintenance, and ease of readership. I'll check if the cache still aids performance, and remove it if not.

@1ucian0 1ucian0 added the Changelog: None Do not include in the GitHub Release changelog. label Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance regression from disabling sympy evaluate

3 participants