Bugfixes and efficiency improvements for basis conversion, inner products, and computing Frobenius distance #432

rileyjmurray · 2024-04-25T12:59:47Z

Changes:

Remove frobeniusnorm and frobeniusnorm_squared functions (which were only correct for real inputs). Inline equivalent numpy functions in the few places they were used.
Replace the np.trace(np.dot(...)) pattern for computing the trace inner product with np.vdot(...). This resolves Replace instances of np.trace(np.dot(... when computing the trace inner product or Frobenius norm. #430.
Note that use of np.vdot has a secondary affect of conjugating the first argument when dealing with complex inputs. This resolves a limitation of basis conversion discussed in an email thread titled Re: Possible pyGSTi bugs w/rt converting between matrix bases? that Robin started on Feb 23 of this year.
I rewrote our implementation for computing state fidelity. It no longer needs the _hack_sqrtm function. It also has $O(n^2)$ time checks for the fidelity formulas when one of the arguments is rank-1. The overhead of these checks might actually degrade performance for 2-by-2 matrices, but for anything bigger we should be fine. If the overhead in the 2-by-2 case becomes an issue down the line then we can special-case that codepath.

Old text

Some tests are failing. I'm not sure why. Maybe they're related to the conjugation that's applied when using vdot in the basis conversion? If so, I would guess that the failing tests are comparing to a nominal value which actually was not correct to begin with. Thoughts, @coreyostrove, @sserita, @enielse?

FAILED test_packages/extras/test_interpygate.py::InterpygateTestCase::test_timedep_factory - AssertionError: 0.005780277171757022 != 0 within 7 places (0.005780277171757022 difference)
FAILED test_packages/extras/test_interpygate.py::InterpygateTestCase::test_timedep_op - AssertionError: 0.00048145808272016756 != 0 within 7 places (0.00048145808272016756 difference)
FAILED test_packages/extras/test_interpygate.py::InterpygateTestCase::test_timeindep_factory - AssertionError: 0.005780277171757022 != 0 within 7 places (0.005780277171757022 difference)
FAILED test_packages/extras/test_interpygate.py::InterpygateTestCase::test_timeindep_op - AssertionError: 0.00048145808272016756 != 0 within 7 places (0.00048145808272016756 difference)

-- No one was able to reproduce these errors (including me, later on!).

…not needed and which were only correct for real inputs). Replace the np.trace(np.dot(...)) pattern for computing the trace inner product with np.vdot(...). Use of np.vdot has a secondary affect of conjugating the first argument when dealing with complex inputs, which resolves a limitation of basis conversion discussed in an email thread.

pygsti/tools/basistools.py

pygsti/tools/jamiolkowski.py

pygsti/tools/matrixtools.py

test/unit/tools/test_optools.py

pygsti/report/reportables.py

sserita · 2024-04-25T15:56:48Z

Haven't done a full review yet, but I think I have the most familiarity with the interpygate code so I will put that on my todo list. The interpygate tests are much closer to integration tests than true unit tests, so it is possible the nominal value was wrong, but needs more investigation.

coreyostrove

Thanks for the great work, @rileyjmurray! I've left a few minor questions for you as part of my review, but by and large these changes all look good, and should bring some performance gains to boot!

pygsti/baseobjs/basis.py

pygsti/modelmembers/operations/__init__.py

pygsti/tools/optools.py

test/unit/tools/test_basisconstructors.py

coreyostrove · 2024-04-30T22:19:25Z

As for the failing unit tests you've encountered, I pulled the latest version of this branch and tested this locally on my machine and was unable to reproduce this behavior. That would likely point to this either being a non-deterministic error, or else something configuration dependent in which case we'd need to track this down a bit more. One bit of low-hanging fruit we can try to eliminate is whether this is somehow macOS specific, so I'll manually launch the mac runners on github to see if we can identify anything there.

coreyostrove · 2024-05-01T16:29:28Z

Update: I ran the unit tests in question on the runners, and with the exception of the MacOS runner for python 3.8 (which failed because of perennial problems with compiling CVXOPT that typically fix themselves after a while) these all passed. So whatever is going on it seems to be localized to the particular combination of packages/configuration on your machine, @rileyjmurray.

Concerns about potential testing issues was the main reason why I didn't mark this as approved when doing my review, so I'm going to switch that designation now that I'm seeing these passing. The other question I had I am still interested in, but these aren't blocking concerns/questions in any way.

…_optools.py::GateOpsTester

coreyostrove

Added a few more comments and questions related to your most recent changes.

pygsti/baseobjs/basis.py

pygsti/modelmembers/operations/fullcptpop.py

pygsti/tools/optools.py

test/unit/tools/test_optools.py

rileyjmurray · 2024-05-07T01:54:04Z

@sserita, I've gotten Corey's sign-off. Can you approve and merge?

sserita

Thanks @coreyostrove and @rileyjmurray for the great work :)

rileyjmurray requested review from a team as code owners April 25, 2024 12:59

rileyjmurray requested review from sserita and removed request for a team April 25, 2024 12:59

rileyjmurray changed the base branch from master to develop April 25, 2024 13:00