Skip to content
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

Bugfix forced germs #292

Merged
merged 7 commits into from
Jan 19, 2023
Merged

Bugfix forced germs #292

merged 7 commits into from
Jan 19, 2023

Conversation

coreyostrove
Copy link
Contributor

This PR adds bugfixes related to the handling of the 'force' argument in the germ selection algorithms. Three different issues were identified and have been patched:

  1. There was no support for the case where force=None, and as such the initial germ set fed into the greedy search loop was empty. This case is now fully supported, with the algorithm doing a greedy initialization (choosing the germ from the candidate list with the highest score) to select the first germ.
  2. When passing in a user-specified list of germs for the value of force an edge case arose when, for various possible reasons, the user-specified list included circuits that were not part of the search space. There is now logic to detect when this happens and explicitly add these extra germs to the search space if required.
  3. There is now an extra check, following the initial germ set construction but before the main optimization loop, for whether the initially constructed germ set is already itself amplificationally complete. Before, the first time this was checked was at the end of the first iteration of the greedy search loop, meaning that you'd always go through one round of greedy search (and add one additional germ to the set) before checking whether the set was AC for the first time. Most of the time this is fine in practice, since usually the initial set isn't AC, but an edge case that broke in the interpygate test suite highlighted that this wasn't what would be considered expected behavior. If the initial set is AC we now return that set and short-circuit the rest of the search. This check can be disabled with a kwarg passed to the greedy search algorithm, which can save some computation time when a users knows the initial set won't be AC.

Finally, there are a couple extra unit tests to cover cases 1 and 2 above.

Corey Ostrove added 7 commits January 5, 2023 15:22
Patches a problem that arises sometimes when using a user-specified list of germs together with deduping. The code expects that the user specified list will be included in the list of available candidates, which after deduping isn't always the case. This can also fail if the settings used to construct the candidate germ set are incompatible with the properties of the forced germ set (either because they are too short or maybe only a random subset of a particular length were initially included etc.) This patches that be adding a check for whether all of the forced germs are in the candidate set and if not appends the requisite ones.
This commit adds support for starting the greedy search from scratch as is required when the 'force' argument is set to None (as opposed to 'singletons' or a user-specified list. In this case we now iterate through the candidate germ list and select the best germ according the the composite score.
Previously the first outer iteration of the greedy search would mistakenly report that the initial germ set amplified only a single parameter (which is incorrect in general). This is because the number of amplified parameters is only available following the initial outer iteration. We could add in a calculation to get the correct number for the initial germ set at the start of the search, but this is computationally expensive, so I went with the cheap route and added a flag so that we simply don't report the number of amplified parameters until we've done at least one outer-iteration of the greedy search.
Revert a change that was added to help patch the issue with the lack of support for initializing the greedy search from scratch. Now that this is fully supported these lines are no longer needed.
Adds a test (along with a flag for disabling this for expediency) that checks the initially constructed germ set, either from the argument passed in for 'force' or as greedily selected when force is None, for amplificational completeness using the test_germs_list_completeness function. If it is already AC then we short-circuit the search and simply return the initial list and print logging information to that effect.
Adds a bugfix for the initial number of eigenvalues. See comment for more details.
Adds additional unit test covering the case where force=None and the initial germ set is selected greedily. Also adds a test covering the case where a germ in a user specified list of forced germs is outside the candidate set, either because it is outside of the specs (i.e. longer, not sampled randomly etc.) or because it may have been originally but dropped during the de-duping.
Copy link
Contributor

@sserita sserita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good, except we can make the kwarg match fidsel for the pretest.

pygsti/algorithms/germselection.py Show resolved Hide resolved
Copy link
Contributor

@sserita sserita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this bugfix!

@sserita sserita merged commit 8d86182 into develop Jan 19, 2023
@sserita sserita deleted the bugfix-forced-germs branch January 19, 2023 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants