add support for Parameters to SparseLabelOp

[kevinsung]

Summary

Part of #828

Details and comments

Also fixes #892

[coveralls]

Pull Request Test Coverage Report for Build 3364057348

• 47 of 47 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 85.823%

---

Totals
Change from base Build 3362454253:	0.03%
Covered Lines:	17441
Relevant Lines:	20322

---

💛 - Coveralls

[mrossinek]

Thank you, Kevin!

Besides these minor comments just a question: did you happen to also look into how the PolynomialTensor class can handle Parameter objects? In theory, this should be possible and I had done some rough testing a while back but not looked into it in more detail.

We can add support for that as part of another PR and a separate tracking issue, but I wanted to ask first, whether you had already looked into this 👍

[kevinsung]

did you happen to also look into how the PolynomialTensor class can handle Parameter objects? In theory, this should be possible and I had done some rough testing a while back but not looked into it in more detail.

I did not look into it in much detail. Let's leave that as a separate issue and separate PR. I think using Parameters with PolynomialTensor is a very niche use case so it is not a high priority. PolynomialTensor is mainly useful for linear algebra operations, and those would not be possible if it had Parameters.

[mrossinek]

I think using Parameters with PolynomialTensor is a very niche use case so it is not a high priority. PolynomialTensor is mainly useful for linear algebra operations, and those would not be possible if it had Parameters.

Quite on the contrary. Enabling parameterized PolynomialTensor objects could indeed be very powerful.
Plus, linear algebra would also not be a problem, since the Parameter objects do support numerics just fine (otherwise this would also not work for the SparseLabelOp).

One example where a parameterized PolynomialTensor would be useful is the refactoring of the ooVQE algorithm, which applies parameterized orbital rotations.

Just a proof of concept:

import numpy as np

from qiskit.circuit import Parameter
from qiskit_nature.second_q.operators import FermionicOp, PolynomialTensor

a = Parameter("a")
b = Parameter("b")

mata = np.eye(2, dtype=object)
mata[0, 0] = a
matb = np.zeros((2, 2), dtype=object)
matb[0, 1] = b
matb[1, 0] = -b

pt_a = PolynomialTensor({"+-": mata})
pt_b = PolynomialTensor({"+-": matb})
print(pt_a + pt_b)
print(pt_a @ pt_b)
op = FermionicOp.from_polynomial_tensor(pt_a + pt_b)
print(op)
op = FermionicOp.from_polynomial_tensor(pt_a @ pt_b)
print(op)

Outputs (omitting zero-terms because chop needs a little work):

Polynomial Tensor
 "+-":
[[ParameterExpression(a) ParameterExpression(b)]
 [ParameterExpression(-1.0*b) 1]]
Polynomial Tensor
 "+-+-":
[[[[ParameterExpression(0) ParameterExpression(a*b)]
   [ParameterExpression(-1.0*a*b) ParameterExpression(0)]]

  [[0 ParameterExpression(0)]
   [ParameterExpression(0) 0]]]


 [[[0 ParameterExpression(0)]
   [ParameterExpression(0) 0]]

  [[0 ParameterExpression(b)]
   [ParameterExpression(-1.0*b) 0]]]]
Fermionic Operator
number spin orbitals=2, number terms=4
  a * ( +_0 -_0 )
+ b * ( +_0 -_1 )
+ -1.0*b * ( +_1 -_0 )
+ 1 * ( +_1 -_1 )
Fermionic Operator
number spin orbitals=2, number terms=4
a*b * ( +_0 -_0 +_0 -_1 )
+ -1.0*a*b * ( +_0 -_0 +_1 -_0 )
+ b * ( +_1 -_1 +_0 -_1 )
+ -1.0*b * ( +_1 -_1 +_1 -_0 )

[kevinsung]

By linear algebra I meant eigenvalue decompositions and the like, not just matrix multiplication. Isn't that needed for computing orbital rotations?

[mrossinek]

By linear algebra I meant eigenvalue decompositions and the like, not just matrix multiplication. Isn't that needed for computing orbital rotations?

Those would not work, yes. The way that the orbital rotations are done there, I do not think its needed. Although this may not hold generally speaking.

[mrossinek]

Other than this, this LGTM 👍

[kevinsung]

I think this is broken because of a bug introduced by #893. See https://github.com/Qiskit/qiskit-nature/pull/893/files#r998269022.

[mrossinek]

Just one last clarification, other than that this is good to go 👍

[mrossinek]

Thanks, Kevin!

[woodsp-ibm]

This is a more general question. What do we expect of such parameterized operators. If I look at a parameterized quantum circuit I have methods to get parameters and also to bind values etc. Might I want this at this level - I see that you've done similar to SparsePauliOp but I guess I could ask the same question of that too.

[kevinsung]

Ok, I've added the method. Also, in order to support assigning parameters in place, I changed the type of the internal data container from Mapping to dict.

[mrossinek]

Also, in order to support assigning parameters in place, I changed the type of the internal data container from Mapping to dict.

I do not understand why you think this is necessary. The public API can report Mapping just fine. The fact that we use a dict internally for storage does not affect this.

[kevinsung]

I do not understand why you think this is necessary. The public API can report Mapping just fine. The fact that we use a dict internally for storage does not affect this.

When copy=False, previously the type of the internal storage object could be Mapping rather than dict. This precludes in-place operations because Mapping does not support item assignment. I updated the internal storage to always be dict. But if the input to __init__ is Mapping then we can't support copy=False. So I changed the input type to also be dict.

[mrossinek]

Alright I have also discussed with @woodsp-ibm offline. We would like to do the following:

• use Mapping for all public-facing API which is aligned with the immutable operator idea and allows users to specifically provide immutable data if they want to further enforce this
• when inplace=True is requested by the user (which we want to support to avoid additional copies if a user so desires), proceed with an isinstance(..., MutableMapping) check. If this check fails, raise a TypeError. Otherwise proceed normally since the user specifically requested inplace=True

[kevinsung]

In accordance with our offline discussion, I've reverted the change from Mapping to dict and removed the inplace argument to assign_parameters. We will make SparsePauliOp mutable in the future; this is tracked at #917.

[mrossinek]

Just some minor comments. Other than this, this is looking good 👍

[mrossinek]

This LGTM 👍

[kevinsung]

Is there anything else? If not I'll go ahead and fix the merge conflicts.

[woodsp-ibm]

Is there anything else?

Not from me, let's get the conflict sorted then.

[mrossinek]

LGTM, thanks Kevin!