Failing Unit Test Bugfixes and Performance Related Testing Refactors #372
Conversation
I am unable to reproduce the failure scenario locally on my windows machine, so try some remote debugging on the github runners.
The streamlines the tests in the algorithms test module and does a refactor of the test fixtures to use a more modern modelpack format. Also improves coverage on fiducial selection codebase.
Add the failing line of the germ selection notebook as a unit test to try and get more verbose output on the failure from the runner. Also temporarily tweak main.yml to only test against windows for faster turnaround.
Some more verbosity for debugging
Try using the schur decomposition in the twirling superoperator construction instead. Also some unrelated fixes for the fiducial pair reduction unit tests.
…e-faster-algorithm-tests
To use a more modern modelpack, and also a smaller one which speeds up the testing a bit.
Streamlined the RB unit tests to test on a fewer number of qubits for a modest reduction in the runtime.
Change the distribution CLI option for xdist to ensure tests remain properly grouped across workers.
Reduce the number of parameterizations fit in the QutritGST demo notebook to reduce runtime (it was periodically timing out on certain cells on the github runners).
Update the implementation of the twirling superoperator to check the commutator of the input matrix and branch off of that to determine the decomposition to use. Also rename the new germ selection unit test.
Resume testing against the entire module.
Now that the evotype dependent serialization bug is resolved go ahead with disabling checkpointing on these tests.
See above title.
There was a problem hiding this comment.
Mostly looks good! I have two substantive comments for the eig vs Schur issue. I also have a question about the level of user-exposure for code in pygsti/algorithms/directx.py.
Somewhat tangentially, I stumbled across this comment block for the Label class, which says that it's supposed to be a base class only. Based on how the Label class is used in practice, it seems like this comment is wrong. Corey, what's your take?
| sigma, | ||
| Label('GsigmaLbl') if sigma.line_labels == ('*',) else Label('GsigmaLbl', sigma.line_labels), | ||
| dataset, prep_fiducials, meas_fiducials, target_model, |
There was a problem hiding this comment.
Interesting that you've made a change to this file, since Erik just marked it as minimal-exposure. @coreyostrove any chance you'd disagree with Erik's take on this file's user exposure? Here's the conventions we agreed to, for reference:
- High exposure: API changes are likely to break workflows for some users, including "casual" users.
- Low exposure: API changes are unlikely to affect casual users of pyGSTi but might affect some of pyGSTI's advanced users.
- Minimal exposure: while it's conceivable that API changes could break user code, the possibility of this is sufficiently remote that we can change the API at will.
There was a problem hiding this comment.
Good question. I agree with Erik's marking of this as minimal-exposure. This change arose as a bugfix for some failures encountered in the corresponding test package (test_directx.py) as part of the refactor moving from legacy modelpacks to the modern implementation in testing. This particular line of code turned out to be fragile when it came to circuit line labeling, and so broke when using circuits from the new modelpacks that have different default circuit line labeling behavior than they do in the legacy modelpacks.
In reality I suspect this is likely due to a disjoint bug in the circuit line label handling code itself in the circuits module that I've run into in other contexts and mentioned to @sserita before, but haven't ever properly documented. (This is a bug related to properly reconciling line labels between circuits with a placeholder '*' and those with fully specified labels). In the moment it was easier to patch in a workaround in directx than to context switch and try to track the circuit bug. That said, I am in the correct headspace for this sort of thing now, so if nothing else I should be able to get a minimal reproduction of the aforementioned bug documented pretty soon.
While I was working in the directx code I did wonder to myself if we even need any of this anymore? I.e. insofar as it is minimal exposure I suspect that is because few (possibly 0) people use it in practice and there are now generally better (more pygstithonic) ways to produce the same results. I've personally never interacted with this code in practical usage, and don't know of anyone who does use this code. Given that, maybe this would be a decent candidate for a module we can retire to legacy as part of out spring cleaning process. @enielse and @sserita, thoughts on this last question?
There was a problem hiding this comment.
Update: I just read through @enielse's latest commit on the code marking branch and saw that he has marked directx as:
user-exposure: minimal, forsaken (EGN - we don't use this anymore and never really did)
So that is two votes in favor of decommissioning this part of the codebase. @sserita, how do you feel about formally marking this as deprecated in 0.9.12.0 and then removing it (remove = move to the legacy folder) either in 0.9.13.0 or some point thereafter?
There was a problem hiding this comment.
I agree with the minimal tag and we can formally mark this as deprecated. I will do so in the bugfix-for-0.9.12 branch before I merge that in.
This commit adds a new kwarg for specifying the tolerance to use in determining whether a matrix is normal for the purposes of selecting the correct decomposition algorithm. Also adds a proper docstring explaining the various function arguments and outputs and removes some holdover print statements from debugging.
|
RE: Label. I use the base Label class directly pretty regularly, for what that is worth. I suspect that I am not alone in that regard, and that the comment block you mentioned probably accurately describes the initial design intentions, but not present behavior. |
This PR includes two main changes:
It looks like all of the unit tests, including the extras and the notebook regression tests (which I manually dispatched on this branch) are passing now. Let me know if you have any questions.
One thing worth noting, I had originally merged in the branch
bugfix-for-0.9.12.0, but found that this resulted in a number of unit tests in the extras package failing. Check out 'build and run test extras' 211 for more details on that. So I opted to revert that merge on this branch to keep the changes separate (and save myself the effort of tracking that down).