Fix setter so that SparsePauliOp.paulis.phase stays zero

[aeddins-ibm]

Summary

Goal is to fix #12883.

Currently several SPO methods assume that SPO.paulis.phase is all zero. However nonzero phase is allowed by the paulis() setter, leading to incorrect results when those other methods are called.

Details and comments

In this small PR we update the paulis() setter to absorb the phase into the SPO coeffs.

[coveralls]

Pull Request Test Coverage Report for Build 10389703463

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.007%) to 89.674%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	2	91.98%

Totals
Change from base Build 10388915881:	0.007%
Covered Lines:	67505
Relevant Lines:	75278

---

💛 - Coveralls

[aeddins-ibm]

Trying to decide between a few approaches.

Approaches I'm leaning away from:

1. Copy input_paulis before assigning to self.paulis. This has some overhead, and is a change from the current setter which does not copy. And it does not leave a public way to set self.paulis without copying.
2. Current state of this PR: Don't copy the input, and also don't mutate the input. Make self.paulis.x (z) the same object as input_paulis.x (z) going forward. But self.paulis.phase will not be the same object as input_paulis.phase. This could lead to weird, subtle bugs as some later operations will update both self.paulis and input_paulis, while others update only one of them.

Proposed new approach is to mutate the input and raise a warning:

if np.any(input_paulis.phase):
    warnings.warn(warning_message, UserWarning) 
    self.coeffs *= (-1j) ** input_paulis.phase
    input_paulis.phase = 0
self.paulis = input_paulis

I think that SparsePauliOp(input_paulis, copy=False) basically does this mutation currently, so there is some precedent for it. But it seems more surprising here, where we mutate what's on the RHS of an =. So I think it's worth including the warning if input_paulis.phase is nonzero.

Alternatively we could raise an error when input_paulis.phase is nonzero, but I think it's better to support the ability to assign a phased PauliList, as long as the user is aware of what's happening. An error would also be more disruptive for existing code.

I don't have experience making this kind of call in a large codebase, so am very open to suggestions, but will try the warning for now.

[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 following people are relevant to this code:

• @Qiskit/terra-core

[Cryoris]

Nice fix! Just two small comments below.

[Cryoris]

Since the initializer also silently maps the phase to the coefficients, I think we don't need a warning here, as this is the standard behavior. Rather, the setter was inconsistent with the initializer 🙂

[aeddins-ibm]

Got it, thanks for the feedback on this.

I'm still a little concerned users may get burned if they try to use the input PauliList for other stuff after the assignment, like

spo_storing_interim_result.paulis = paulis_with_phase
final_result = some_function_that_depends_critically_on_phase(paulis_with_phase)

but agree that 1) this PR is consistent with the initializer, 2) the above example is a somewhat unusual code structure, 3) as Jake mentioned on the issue page, this PR is kind of a band-aid since really SparsePauliOp should not have a phase attribute at all.

I'll go ahead and remove the warning per your suggestion.

[jakelishman]

Andrew: I'd suggest that that code example is a problem outside SparsePauliOp - after the line spo_storing_interim_result.paulis = paulis_with_phase, it's not valid to make any assumptions about the content of paulis_with_phase, because the SparsePauliOp owns the object. I think in general, users can hopefully either re-order the functions so that they still haven't given away ownership of the object while they're using it, or they explicitly copy it if they're still using it.

Thanks for thinking about this, though - it's good to have performance and/or mutability concerns in mind when dealing with Python.

[Cryoris]

Could you also add a test to check the simplify() behavior you described in your original issue?

[aeddins-ibm]

Added.

[Cryoris]

Thanks for the fix! 🙂