Parameter handling in SparsePauliOp

[ikkoham]

Summary

Takeover #7215.

Details and comments

TODO:

• Tests
• Releasenote

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Qiskit/terra-core
• @ikkoham

[coveralls]

Pull Request Test Coverage Report for Build 3140920284

• 22 of 22 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.002%) to 84.469%

---

Totals
Change from base Build 3139567056:	0.002%
Covered Lines:	59957
Relevant Lines:	70981

---

💛 - Coveralls

[jakelishman]

The dtype=object functionality doesn't feel good to me, but that's not for me to decide, and I can see you've taken care to make sure it doesn't affect the runtime for the existing functionality.

This will need documentation and tests of all the new functionality adding before, if it's moving towards merge.

edit: ha, I just saw that this is the continuation of a previous PR, which I apparently reviewed before (and didn't remember at all...). This looks like a much more targetted PR, and the speed implications on the existing cases do look far more minimal. Could you also run the SparsePauliOp asv benchmarks on this PR to compare them to main before it? It'd be good to verify that there's little-to-no performance impact.

[jakelishman]

This is pretty hard to read, and it causes objects that trigger complex(coeff.sympify()) to get treated differently to other complex numbers. You might want to refactor the inner part into a function, or do something like

def to_complex(coeff):
    if not hasattr(coeff, "sympify"):
        return coeff
    sympified = coeff.sympify()
    return complex(sympified) if sympified.is_Number else np.nan

non_zero = np.logical_not(np.isclose([to_complex(x) for x in self.coeffs], 0, atol=atol, rtol=rtol))

to guarantee consistency. Speed probably isn't a factor, since object dtype arrays lose all the vectorisation benefits anyway (I strongly imagine, since they'll need to acquire the GIL and run in Python space, but I haven't actually tested).

[ikkoham]

Thank you for your suggestion. reflected in d01b52e.

[jakelishman]

Similar comment to above.

[mrossinek]

Besides Jake's previous comments I agree that this needs documentation + test updates. Other than that, this seems to be exactly what we will need to also support parameterized operators on the second-quantization level in Qiskit Nature.

@ikkoham anything you need help with here, to ensure this PR makes it into 0.22?

Tagging @kevinsung

[ikkoham]

Yes, it is up for discussion whether this is the best implementation, but we need the Parameterized SparsePauliOp as a new feature for 0.22. I will do my best to check performance and add more tests and documentations.

[jakelishman]

The body of the changes looks good to me, thanks for taking care with the timings of the other paths. I pushed up a commit to reword some of the documentation and expand the release note, but nothing in the actual implementation.

I had a couple of comments about how we might make the tests a bit easier to deal with - they're a bit mysterious to me right now.

I know @mtreinish also wanted to have a look through this before merge to know how this might affect accelerating some SparsePauliOp routines with Rust.

[ikkoham]

Probably it's not a problem, since numpy in Rust can have PyObjects as elements.

[jakelishman]

This looks good from my perspective, thanks for the quick changes! I'll leave it untagged for now so Matthew has a chance to look as well if he wants, and if not, he can just tag automerge.

[mtreinish]

Probably it's not a problem, since numpy in Rust can have PyObjects as elements.

The bigger issue with this is that to do any numeric operations on an ndarray with an object dtype in rust is that we'll need to callback to python to use python's functions which kind of defeats the purpose of doing anything from rust because we'll end up being bound by python's performance. But it's not really a blocker because I expect for any implementation of #8772 we'll keep the python class and can just dispatch to rust if it's a complex dtype and leave the python code for the object dtype.

[mtreinish]

This LGTM, I think this will be easy to manage eventually using rust for SparsePauliOp we can just check the dtype where needed like we already do in other places. This makes the split reasonable to manage for 0.23.0 (assuming we want to tackle it there).

[mtreinish]

I might change this to say object dtype sparse pauli ops don't support the following methods. Just because I could envision a use case where a different custom class is used. (thinking directly using symengine/sympy symbols or maybe some other libraries types. But it's not a big deal (and docs can be updated later too).