Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Handle symengine based parameter expressions #1586

Merged
merged 5 commits into from
May 5, 2021

Conversation

mtreinish
Copy link
Contributor

Summary

In Qiskit/qiskit#6270 symengine is being added as an optional (but
default on common platforms) backend for parameters and parameter
expressions. However, the aqua version of the gradient code there is an
implicit assumption that parameterexpressions are internally just wrappers
of sympy expressions. This assumption about the internals of terra breaks
with Qiskit/qiskit#6270 where they might be symengine or sympy
expressions. While symengine and sympy expressions are interchangeable
this can only be done with an explicit conversion step (for example,
sympy.sympify(symengine.Symbol('x'))). This commit fixes this by
updating the derivative base class as was done for terra's version in
Qiskit/qiskit#6270 so that the deprecated aqua copy continues to
work wither versions of terra after Qiskit/qiskit#6270 merges.

Details and comments

In Qiskit/qiskit#6270 symengine is being added as an optional (but
default on common platforms) backend for parameters and parameter
expressions. However, the aqua version of the gradient code there is an
implicit assumption that parameterexpressions are internally just wrappers
of sympy expressions. This assumption about the internals of terra breaks
with Qiskit/qiskit#6270 where they might be symengine or sympy
expressions. While symengine and sympy expressions are interchangeable
this can only be done with an explicit conversion step (for example,
`sympy.sympify(symengine.Symbol('x'))`). This commit fixes this by
updating the derivative base class as was done for terra's version in
Qiskit/qiskit#6270 so that the deprecated aqua copy continues to
work wither versions of terra after Qiskit/qiskit#6270 merges.
@mtreinish mtreinish added the status: on hold E.g. needs more discussion or more information (preformance, tests, ...) label Apr 22, 2021
@mtreinish
Copy link
Contributor Author

I'm labeling this as on hold until we have agreement that Qiskit/qiskit#6270 is going to happen (I assume it will because the performance improvement is significant, but we haven't had the discussion yet). Once there is agreement this will need to merge to unblock CI on Qiskit/qiskit#6270.

Cryoris
Cryoris previously approved these changes Apr 22, 2021
@mtreinish mtreinish removed the status: on hold E.g. needs more discussion or more information (preformance, tests, ...) label May 5, 2021
@mtreinish
Copy link
Contributor Author

This is unblocked now, Qiskit/qiskit#6270 has been approved. We need this fix to unblock the terra PR now.

The one open question in my head is do we want to somehow guard against a potential situation where someone has aqua >=0.1.0 installed and symengine installed by terra <0.18.0 ? Because right now if someone installs aqua master with symengine but is using terra 0.17.x I think this will fail (because it's building a symengine expression instead of a sympy expression like what is unconditionally used in terra 0.17.x)

manoelmarques
manoelmarques previously approved these changes May 5, 2021
@manoelmarques manoelmarques merged commit 6fedc30 into qiskit-community:main May 5, 2021
manoelmarques added a commit to manoelmarques/qiskit-aqua that referenced this pull request May 5, 2021
* Handle symengine based parameter expressions

In Qiskit/qiskit#6270 symengine is being added as an optional (but
default on common platforms) backend for parameters and parameter
expressions. However, the aqua version of the gradient code there is an
implicit assumption that parameterexpressions are internally just wrappers
of sympy expressions. This assumption about the internals of terra breaks
with Qiskit/qiskit#6270 where they might be symengine or sympy
expressions. While symengine and sympy expressions are interchangeable
this can only be done with an explicit conversion step (for example,
`sympy.sympify(symengine.Symbol('x'))`). This commit fixes this by
updating the derivative base class as was done for terra's version in
Qiskit/qiskit#6270 so that the deprecated aqua copy continues to
work wither versions of terra after Qiskit/qiskit#6270 merges.

* fix lint

* Check for existence of symengine handling in Terra

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>
@mtreinish mtreinish deleted the symengine-compat branch May 6, 2021 16:40
manoelmarques added a commit that referenced this pull request May 6, 2021
* Handle symengine based parameter expressions (#1586)

* Handle symengine based parameter expressions

In Qiskit/qiskit#6270 symengine is being added as an optional (but
default on common platforms) backend for parameters and parameter
expressions. However, the aqua version of the gradient code there is an
implicit assumption that parameterexpressions are internally just wrappers
of sympy expressions. This assumption about the internals of terra breaks
with Qiskit/qiskit#6270 where they might be symengine or sympy
expressions. While symengine and sympy expressions are interchangeable
this can only be done with an explicit conversion step (for example,
`sympy.sympify(symengine.Symbol('x'))`). This commit fixes this by
updating the derivative base class as was done for terra's version in
Qiskit/qiskit#6270 so that the deprecated aqua copy continues to
work wither versions of terra after Qiskit/qiskit#6270 merges.

* fix lint

* Check for existence of symengine handling in Terra

Co-authored-by: Manoel Marques <Manoel.Marques@ibm.com>

* Adjust unit tests for symengine (#1592)

Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants