-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature ddist wildcard integration #267
Conversation
Adds a new wildcard budget object that inherits from PrimitiveOpsWildcardBudget, but also stores the relevant diamond distances and alpha parameter for the diamond distance wildcard model. Also, minor typo fix in PrimitiveOpsWildcardBudget.
Adds in the ability to generate diamond distance wildcard models as a badfit option in GST results.
Adds a new plot in pgysti reports that shows the diamond distance and wildcard budgets for the new diamond distance wildcard model.
Changes verbiage for the wildcard model's alpha parameter.
The target estimate is incompatible with the diamond distance wildcard model, so this adds some logic to skip the wildcard analysis on that estimate when present.
Flesh out the docstring a bit for the new diamond distance wildcard option, other parts of this docstring still need some TLC though.
RE: failed unit tests. Took a look at the logs and the four failed tests are all due to that qibo interface change we fixed. This branch was forked off of master so doesn't have that fix integrated. |
Looks like everything is passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this because I think it looks fine, but there are still some minor things we could clean up before merging it in (print statements and docstrings).
pygsti/protocols/gst.py
Outdated
right = None | ||
|
||
while left is None or right is None: | ||
print(f'Searching for interval [{left}, {right}] with guess {guess}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to make this just debugger/info logging rather than print all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, just pushed a commit implementing that.
pygsti/protocols/gst.py
Outdated
redbox_threshold = _chi2.ppf(1 - percentile / nboxes, 1) | ||
|
||
# Not in cache, recompute | ||
ddists = _compute_ddists(estimate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just point for discussion for tomorrow's dev meeting: I could imagine the user wanting to do this sort of scaling single-param wildcard with something other than the diamond distance. Is it worth plumbing that in now? Could be as simple as adding a kwarg to this function, and we can leave the rest of the plumbing for when we need it?
Moves some printed output into a proper verbosity printer object in bisect_alphas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me on the whole. We still may want to change the ddist
name in various places and hardcoded diamond distance to be more flexible before merging (diamond distance requires CVXPY in particular, which can be grumpy).
pygsti/protocols/gst.py
Outdated
@@ -556,7 +610,7 @@ def __init__(self, threshold=DEFAULT_BAD_FIT_THRESHOLD, actions=(), | |||
wildcard_L1_weights=None, wildcard_primitive_op_labels=None, | |||
wildcard_initial_budget=None, wildcard_methods=('neldermead',), | |||
wildcard_inadmissable_action='print'): | |||
valid_actions = ('wildcard', 'Robust+', 'Robust', 'robust+', 'robust', 'do nothing') | |||
valid_actions = ('wildcard', 'ddist_wildcard', 'Robust+', 'Robust', 'robust+', 'robust', 'do nothing') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe '1d_wildcard'?
pygsti/protocols/gst.py
Outdated
# Iterate through alphas and compute wildcard feasibilities | ||
feasible = [] | ||
for alpha in alphas: | ||
wcm = _ddist_wildcard_model(alpha, ddists) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note: we could avoid creating a new budget object on each iteration here if we wanted/needed the performance. I think this is fine for now - just an opportunity for later on if performance becomes an issue.
Thanks for adding that additional docstring information, @enielse. I think I will work on generalizing this to work with more error metric in preparation for the next release, so I'll wait to merge this into develop until then. |
…metrics. Renames 'PrimitiveOpsDiamondDistanceWildcardBudget' to 'PrimitiveOpsSingleScaleWildcardBudget' and also refactor the base PrimitiveOpsWildcardBudget class so that the new wildcard budget has just a single parameter (alpha) while still retaining the useful machinery of the base class. Updates 'ddist_wildcard' to 'wildcard1d' as a bad-fit option when running GST, and adds a 'wildcard1d_reference' attribute to GSTBadFitOptions. By default, badfit_options.wildcard1d_reference == 'diamond distance' and the function _compute_1d_reference_values_and_name computes the diamond distance as before. If we want to add other "built-in" options this just amounts to adding cases to this function. Allowing custom user functions could also be done by allowing an object in addition to string values, but let's leave this as future work if it's ever needed. This commit also moves some of the wildcard optimization code from gst.py into wildcardopt.py, which seems more appropriate, and refactors the wildcard optimization code a bit to lessen the amount of duplicated code. The wildcard bar plot in the report remains diamond-distance specific, and is only triggered when the unmodeled error is a PrimitiveOpsSingleScaleWildcardBudget AND its .reference_name == 'diamond distance'. So, if we want to add additional metric options and report them we'd need to tweak the DiamondDistanceWildcard code in factory.py and figure generation code in section/goodness.py and Goodness_unmodeled.html files where "ddist" and "Diamond Distance" are still hardcoded. Note, however, that we'd also need to change the figure caption & title. The plot class itself, now named WildcardSingleScaleBarPlot, has been generalized to accept a user-specified title for the reference values along with a PrimitiveOpsSingleScaleWildcardBudget.
Ok, I make some updates to the naming and structure that should make it easier to generalize to other metrics in the future if we ever want to. I verified on a simple example that my changes give the exact same wildcard error as the code did before my commit - so hopefully I didn't break anything. |
Awesome work! This all looks great to me and will vastly simplify the addition of new error metrics down the line. |
(picked up by unit tests and grep-checking for old usage.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, except for the one test not passing. Once that is fixed, I think we are good for merge.
Cleans up PrimitiveOpsWildcardBudget a bit in the process.
…io/pyGSTi into feature-ddist-wildcard-integration
This integrates the ability to generate the single-parameter diamond distance wildcard models introduced in this paper (https://arxiv.org/pdf/2207.08786.pdf) and include visualizations of these models in pygsti reports.
I have updated the docstring for GSTBadFitOptions to include an explanation of this new badfit action. This docstring still needs some TLC though, so a hopefully low-effort request for @enielse: any chance you could take a crack at filling in the rest of the blanks on that docstring before this gets merged into develop? I added spots for all of the undocumented options, but I don't actually know what most of them do.