Efficiently check PauliSumOps for diagonality in CVaRMeasurement

[Cryoris]

Summary

Fixes #7573.

Details and comments

See #7573.

[jakelishman]

I didn't realise we had too-many-returns active in pylint, but it's not the end of the world. I can't tag them directly in the diff, but the first few lines can become

if isinstance(operator, PauliOp):
    return not np.any(operator.primitive.x)

and the last lines (if np.all(matrix == ...)) can just become

return np.all(matrix == ...)

That saves two return statements (and is generally just better style), so pylint will stop complaining.

A pre-existing problem, but I notice that SummedOp derives from ListOp, so any SummedOp that isn't short-circuited to True will be iterated through three times; once as a SummedOp, once as a ListOp and then once to build the matrix. The if isinstance(operator, ListOp) could become an elif branch of if isinstance(operator, SummedOp) to prevent that.

[jakelishman]

Modifying a global variable in the test has a knock-on effect on others, because the state leaks out - even if this is the default right now, you should check the previous value, do your test in a try-finally and reset it the variable to its previous value in the finally branch. If the default were ever to change, the correctness of the tests would become order-dependent, and we don't make any guarantees about that.

[mtreinish]

I would actually use self.addCleanup for this instead to have the unittest framework restore the previous value after the test runs regardless of whether passes or fails. Then you don't have to worry about dealing with the error case manually.

[Cryoris]

I don't quite get how one would use addCleanup to reset the value. The only thing I would have come up with is

def restore_algorithm_globals_massive(self, value):
    algorithm_globals.massive = value

def test_cvar_on_paulisumop(self):
    self.addCleanup(self.restore_algorithm_globals, algorithm_globals.massive)
    
    # previous test here

Is that what you had in mind?

[mtreinish]

Yeah, that's more or less what I had in mind. I personally would probably have done something like:

self.addCleanUp(lambda x: algorithms_global.massive = x, algorithms_global.massive)
algorithm_globals.massive = False

but turning the lambda into a function works fine too. addCleanup takes a function and args to call it with and builds a LIFO queue to execute of all the addCleanups calls made during the test execution. In this case it's more straightforward though because you only have a single call and argument with a single function.

[jakelishman]

Another possibility is using unittest.mock.patch as a context manager to patch the value to false for the statements you care about:

with unittest.mock.patch("qiskit.utils.algorithms_global.massive", new=False):
    CVaRMeasurement(...)
    ...

[coveralls]

Pull Request Test Coverage Report for Build 1778963336

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

---

Totals
Change from base Build 1778928343:	0.003%
Covered Lines:	51909
Relevant Lines:	62423

---

💛 - Coveralls