pyGSTi
pyGSTi copied to clipboard
sandialabs
FPR Bugfixes and Feature Enhancements
The changes in this branch include the following:
- Fixes to the EigenvalueParamDenseOp to include as parameters all of the off-diagonal elements of the kite. This fix is of independent interest beyond FPR.
- Modifications and feature enhancements for per-germ and per-germ power FPR.
- Some minor bugfixes to the fisher information calculations in edesigntools (probably should have been made on a separate branch, sorry!).
Since github has indicated that fully automatic merging won't work, my apologies in advance if this pull request is a pain to incorporate into develop.
I just took a look at the logs to see which unit tests were failing and it looks like it is the test for verifying a certain set of fiducials is generated by the FPR algorithm. It looks like this is hardcoded in at the moment and I'm guessing that one of my changes made it so a different set of fiducials is returned now. Should be an easy thing to fix.
Ok, thanks for looking into the tests. I'll take a look at this more closely when everything passes.
Re: failing tests, I just pushed a few fixes for the FPR tests, but they are still failing inexplicably... Strangely they pass on ubuntu with python 3.6, but look to be failing for the new versions of python. I'll keep looking into this.
Following up again on this endless saga of unit testing. As of now all of the unit tests are passing on ubuntu, and so are all of the unit tests related to FPR on windows . The remaining failing tests look to be related to Qibo integration, and the errors only show up on python 3.8+ for some reason. (See the actions logs for commit 3bf5e48 and 1654a70) I can look into this if you'd like (though, @enielse would probably be better suited to do so since it involves Qibo). Either way any unit test fixes related to Qibo are probably out of scope for this PR, and I'd suggest we stick those on a different bugfix branch.
Thanks for your effort on this Corey. I'll do a full review of this PR again later this week, but my 30 second skim does leave me mildly concerned about the Qibo tests. At a high-level, it just seems like a kwarg mismatch - however, we have actions post-Qibo merge where these don't occur (first example). What this means to me is maybe there was an issue merging develop into this branch? Or at least, I would like to double check that this was not the case before final approval.
I was re-running the CI jobs individually to confirm my assertion that the ubuntu jobs were still passing after the latest commits, but they failed on the qibo tests now which I found strange since there were no changes in those commits that could've possibly affected these tests (a kwarg update on a tutorial notebook and an update to the FPR unit tests). Long story short, the qibo devs pushed a new release to pypi on 9/3, and in this new release the seed kwarg has been removed from all of the Channel objects. There wasn't any explanation on the corresponding commit (SHA d182b9b if you're curious) on the rationale for this change, unfortunately.
On our end I'm not familiar with the qibo evotype code, so I'm not sure how essential this functionality was. Either way we'll need to have a separate discussion on how to proceed. I'm guessing we can either update the evotypes on our end to work around this or else update the requirements to use the previous release at the expense of whatever other new features/enhancements were included in this new one. Either way this is outside the scope of this PR so I'll open up an issue thread so we can migrate this discussion there.
I see. I didn't realize it was a Qibo-side change. Thanks for looking into it more in-depth than I did.
Yes, in that case, I agree that it should be moved to a separate thread and either Erik or I will handle it. Incidentally, the proper location of RNG seeding is a problem we also have with other evotypes (e.g. CHP), so it's interesting to see that Qibo changed their interface...