sandialabs / pyGSTi

A python implementation of Gate Set Tomography
http://www.pygsti.info
Apache License 2.0
132 stars 55 forks source link

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

Closed rileyjmurray closed 2 months ago

rileyjmurray commented 2 months ago

Changes:

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!).

sserita commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

rileyjmurray commented 2 months ago

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