FPR Bugfixes and Feature Enhancements #261
Conversation
…rameterized. Previously when constructing a EigenvalueParamDenseOp parameters would be added for degenerate 2x2 blocks by setting include_off_diags_in_degen_2_blocks=True. This, however, was insufficient for correctly performing per-germ-per-L FPR, which needs, e.g., to know about the 3x3 (and larger) degenerate blocks of TP identity/idle gates on 1 or more qubits. This commit renames the argument to include_off_diags_in_degen_blocks and updates the code so that it correctly adds all the parameters needed to parameterize the commutant of the gate matrix it's given (all the off-diagonal elements of a degenerate block).
… and not just rank. Previously a set of fiducial pairs was considered "good" as soon as its rank was equal to the number of (EigenvalueParamDenseOp) parameters that need to be amplified. This may have worked ok, but the new implementation should be significantly more robust: it checks the lowest eigenvalue corresponding to an amplified parameter and selects 1) only fiducial pair lists where this lowest-eigenvalue is above some threshold and where the condition number is less than a threshold and 2) the best among several (minimally 10 or the number of random instances / 2) pair lists. Furthermore, the per-germ-per-L FPR algorithm sets the lowest-eigenvalue threshold for L > 1 cases on the expected increase in sensitivity from lower-L cases already performed. This gives further certainty that the fiducial pairs being selected are as sensitive to the amplifiable parameters as they should be. There are currently a number of HARDCODEed parameters marked in the FPR algorithm that should work fine in most cases, but may need to be plumbed up to being adjustable by the user in the FUTURE.
In particular, this re-freezes the set of per-germ fiducial pairs used in a unit test -- it doesn't create new per-germ-per-L unit tests.
Just increases the memory limit from 30K -> 256K as it was running out of memory. I assume this was caused by the increased number of steps in the per-germ-per-L FPR, but didn't check into it in detail.
Merging develop into bugfix-fpr-pgl-params
…ased on the values from the full fiducial set. Also allows users to modify the minimum number of iterations before the algorithm is allowed to terminate early.
…GSTi into bugfix-fpr-pgl-params
…additional circuits in an edesign FiducialPairPlaquette object in _additional_circuits which aren't picked up by iterating through the plaquettes themselves. These are added during the additional of LGST circuits.
…rch through returned candidate sets for smaller subsets that also work. Note that this will break per-germ-power. Fixes to per-germ power to allow compatibility forthcoming.
…ing the algorithm for per-germ power closer to the current algorithm for per-germ.
In some cases there are collisions between the eigenvalues that was messing with the logic for returning the best solutions. This is a partial fix for that.
This reverts commit c8e9862.
In some cases there are collisions between the eigenvalues that was messing with the logic for returning the best solutions. This is a partial fix for that.
Update per-germ FPR docstring
It was found that in practice the condition number thresholding didn't tell us anything that we didn't already get from the minimum eigenvalue condition, so removing this feature. Continuing to include it when it doesn't seem to do anything extra will most likely lead to confusion.
Unmatched delimeters.
|
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. |
updates the list of fiducials for the FPR unit test.
More unit test fixes.
|
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. |
See if changing the minimum number of iterations and disabling recursive search does anything for us.
Moves a bunch of print statements into proper logging instances with verbosity settings.
Fourth try is the charm.
Fix a minor logging issue.
Move the rank test for insufficient fiducial set Jacobian sooner. I think this should fix the final unit test failure.
Updates the kwargs and discussion in the FPR tutorial notebook to reflect latest algorithm changes.
Try to patch the FPR unit tests to get them passing on windows with python 3.8+
|
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). |
|
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... |
| @@ -11,9 +11,11 @@ | |||
| "\n", | |||
| "where $F_i$ is a \"preparation fiducial\" sequence, $F_j$ is a \"measurement fiducial\" sequence, and \"g_k\" is a \"germ\" sequence. The repeated germ sequence $(g_k)^n$ we refer to as a \"germ-power\". There are currently three different ways to reduce a standard set of GST operation sequences within pyGSTi, each of which removes certain $(F_i,F_j)$ fiducial pairs for certain germ-powers.\n", | |||
| "\n", | |||
| "- **Global fiducial pair reduction (GFPR)** removes the same intelligently-selected set of fiducial pairs for all germ-powers. This is a conceptually simple method of reducing the operation sequences, but it is the most computationally intensive since it repeatedly evaluates the number of amplified parameters for en *entire germ set*. In practice, while it can give very large sequence reductions, its long run can make it prohibitive, and the \"per-germ\" reduction discussed next is used instead.\n", | |||
| "- **Global fiducial pair reduction (GFPR)** removes the same intelligently-selected set of fiducial pairs for all germ-powers. This is a conceptually simple method of reducing the operation sequences, but it is the most computationally intensive since it repeatedly evaluates the number of amplified parameters for en *entire germ set*. In practice, while it can give very large sequence reductions, its long run can make it prohibitive, and the \"per-germ\" reduction discussed next is used instead. \n", | |||
There was a problem hiding this comment.
Just bookmarking for us, but we may want to deprecate/underemphasize global FPR... It's slow and not any better than per-germ.
The changes in this branch include the following:
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.