Fixes for CircuitListsDesign Reporting and Reporting w/o Gauge Optimization #415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 15 commits
January 24, 2023 15:20
A number of the reporting functions for generating figures for reports presently have hardcoded assumptions that the experiment has a PlaquetteGridCircuitStructure associated with it. More general CircuitListsDesign based experiments don't have this sort of structure associated with them. Certain plotting functionality clearly relies on the existence of a plaquette structure, but others like per-sequence model violation histograms and related figures don't. This is an initial round of patches that aim to add more robust support for producing reports for experiments based on CircuitListsDesigns. This adds support for the per-sequence model violation histogram and scatter plots.
…e plots This commit adds some additional modifications to the color box plot code to add additional support/handling for CircuitListsDesign based experiments.
…timization The reports for the results of a ModelTest didn't always produce things like the error generators for a model if there wasn't gauge optimization performed. This adds more robust reporting support for ModelTest results where we don't perform gauge optimization so that the error generators of the non-gauge-optimized model get reported in that case. This is done by adding a trivial gauge optimized model to the estimate that is simply a copy of the model being tested.
Bugfix for the case where the option for defining an n-qubit idle in a processor spec as an integer corresponding to the number of qubits is used.
The conditional that was previously being used for checking whether we should default to target did not look right, as I don't see any reason why we should be checking for the emptiness of the gauge optimization suite in deciding whether to do so given the previous branches of the checks.
Add in a deprecation of the 'none' gauge opt suite name option. This creates two only slightly different code paths/conditions for skipping gauge optimization, with the other being having an empty gauge opt suite, which creates possibilities for errors. Going forward just use the empty GSTGaugeOptSuite object to denote no gauge optimization is performed.
This commit adds better handling for GST without gauge optimization with regards to report generation. This ports over some changes that were made earlier in the context of model testing, another setting in which not having model tests is commonplace. This is done by adding a new model to the estimate called 'trivial_gauge_opt' which is each to 'final iteration estimate' and when detected by the report generation gets subbed in for the generation of gauge dependent results. Also includes some docfixes and removal of debug commands.
I noticed that LinearGateSetTomography was missing auxfile information of the sort that StandardGST and GateSetTomography do for meta directory based serialization. This commit brings this protocol in line with the other two and enables directory based serialization for it. Also adds in a set of unit tests for testing the writing to directory and reading from directory of all three of the aforementioned classes.
Remove 'none' as an officially listed option for the gauge optimization suite.
It occurred to me in writing the PR for this change why there was an option for 'none' as a suite name in the first place, and that is because one can specify a list of different gauge-optimizations to perform, including the case of no gauge optimization. I am guessing, though, that there will probably wind up being a secondary edge case with broken reporting where if you do have a list of multiple gauge opt suites, the gauge variant figures for the 'none' entry will probably not generate properly.
Additional reversion for the changes I made to 'none' handling.
There was a bug present when using multiple gauge optimizations/suites when including 'none' as one of the suboptions wherein the model corresponding to 'none' was not included in the estimate. This bug fix includes a bunch of new edge case handling for when 'none' is included in a list of gauge optimization suites and a number of fixes related to this that ensure it properly shows up in the report.
coreyostrove
changed the title
Fix an edge case bug for where a model is specified for the gauge optimization to add to an estimate and the gauge optimization parameters are not None.
coreyostrove
requested review from
rileyjmurray,
a team and
pcwysoc
as code owners
sserita
requested changes
Apr 2, 2024
There was a problem hiding this comment.
Thanks for this work @coreyostrove! I think it mostly looks good, but just bookmarking a few minor things for us to nail down while we have it fresh in our minds. I am also available to do some of the changes if you agree with them but don't have the bandwidth to do it right now.
pcwysoc
approved these changes
Apr 2, 2024
There was a problem hiding this comment.
Workspaceplots.py largely looks good--I agree with Stefan's comments. There will be some areas that will require modification in order to integrate my model violation fixes for the per sequence box plot, but that's mostly a problem for me merging into my branch for right now. Thanks for working on my report fix, Corey!
sserita
requested changes
Apr 2, 2024
refactor reporting function added to handle CircuitLists to a method for that class instead.
sserita
approved these changes
Apr 15, 2024
This was referenced Apr 16, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR covers changes intended to improve reporting support for two scenarios:
For item 1, previously it was the case that many plots, especially those related to model violation summarization had the assumption that the design has a PlaquetteGridStructure format hardcoded in. This of course makes sense for the model violation and TVD plaquette plots, but is an unnecessary assumption for the per-circuit histogram plots and other model violation statistics plots which would fail to generate. With the changes in this PR we can now generate all of the model violation plots that don't explicitly require this structure.
For item 2, this is in response to the bug report from @pcwysoc in #412. As reported there, when performing GST without gauge optimization the report generation would not create plots/tables related to gauge-variant quantities including distance metric tables, error generator plots, etc... This report reminded me that I ran into this exact same issue over a year ago in the context of generating reports for ModelTest protocols and fixed it on this branch (where I allowed it to subsequently languish after getting distracted). So I've now ported the fix I made for ModelTest over to the GateSetTomography protocol (and pre-emptively LinearGateSetTomography since this would have the same issue if anyone were to use it this w/o gauge optimization).
In the process of working on item 2 I originally implemented some changes to deprecate 'none' as an option, before reverting these changes upon the realization that the utility of having this option (beyond what is available using an empty GSTGaugeOptSuite) is that it allows for including no gauge optimization as an option among other gauge optimization suites (including multiple gauge optimizations in an estimate isn't something we do super often, but is fully supported). In going through this back and forth, however, it occurred to me (spurred on by the intuition about how the code worked that I'd gained from the other bugfixes) that there was going to be an edge case when using multiple gauge optimization suites when including 'none' as one of them wherein this would be skipped/not included in the report. Some subsequent testing confirmed this was indeed the case, with reports produced including all of the selected suites except for None. TLDR, I knew if I didn't fix this now I may never have done so, so I figured out a patch and that fix is included in this PR as well.
Unrelated change: While going through the gst protocol module I happened to noticed that the LinearGateSetTomography protocol was missing the auxfile specifications needed for directory-based serialization of the sort we have for GateSetTomography and StandardGST. So I added those (pattern matching off of the other two), which means that this protocol should be serializable now. Probably not a big deal either way, since I don't think many folks use this with any regularity, but figured I'd do it now since while it was fresh. I also added some new unit tests for all three of these protocols testing the directory-based serialization for each them.