Merge bugfix into develop (resolve failed auto-merge) #675
Conversation
This file contains hidden or 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
Corrects an indexing bug that would cause DataSet lookups to silently fail (return incorrect values) when data had been added with dictionaries that didn't have keys ordered in the same way as the DataSet's outcome list. This commit fixes this issue and adds several unit tests used in debugging.
Corrects an indexing bug that would cause DataSet lookups to silently
fail (return incorrect values) when data had been added with
dictionaries that didn't have keys ordered in the same way as the
DataSet's outcome list. This commit fixes this issue and adds several
unit tests used in debugging.
Previously failing minimal example (now included as a unit test) is:
```
import pygsti
c = "Gxpi2:0@(0)"
counts = {'00': 1, '10': 0, '01': 97, '11': 2}
ds = pygsti.data.DataSet(outcome_labels=["00", "10", "01", "11"], static=False)
ds.add_count_dict(c, counts)
check = ds[c].counts
print(ds)
assert {k[0]: v for k,v in check.items()} == counts
```
This patches another shared memory bug in `lsvec`, this time a race
condition. The bug is easiest explained by pointing at the relevant
lines. Below is the implementation of `lsvec` for
`TimeIndependentMDCObjectiveFunction`:
```
lsvec = self.terms(paramvec, oob_check, "LS OBJECTIVE")
if _np.any(lsvec < 0):
bad_locs = _np.where(lsvec < 0)[0]
msg = f"""
lsvec is only defined when terms is elementwise nonnegative.
We encountered negative values for terms[i] for indices i
in {bad_locs}.
"""
raise RuntimeError(msg)
unit_ralloc = self.layout.resource_alloc('atom-processing')
shared_mem_leader = unit_ralloc.is_host_leader
if shared_mem_leader:
lsvec **= 0.5
if raw_objfn_lsvec_signs:
if shared_mem_leader:
raw_lsvec = self.raw_objfn.lsvec(self.probs, self.counts, self.total_counts, self.freqs)
lsvec[:self.nelements][raw_lsvec < 0] *= -1
unit_ralloc.host_comm_barrier()
```
The bug arises due to the interation of the positivity check ` if
_np.any(lsvec < 0)` and later on the update of `lsvec` using the
`raw_objfn` signs `lsvec[:self.nelements][raw_lsvec < 0] *= -1`. Since
the positivity check is happening on all ranks, but is with respect to
shared memory, if a rank that is the shared memory leader hits and runs
that sign update line before another rank looking at the same memory
hits the positivity check this would raise an error.
After staring at this code for a bit I couldn't see any reason why this
positivity check (or anything else in this function) should be happening
on anything other the shared memory leader, so the patch is to just add
a corresponding if statement which enforces that.
Thanks to @juangmendoza19 for reporting this error.
God I hate shared memory sometimes...
Co-authored-by: Corey Ostrove <[email protected]>
This resolves #644 by making changes to the _create_objective_fn helper called by gaugeopt_to_target. The fix was easier than I expected for two reasons. First, gauge optimization w.r.t. the Frobenius norm already incorporated instruments by way of ExplicitOpModelCalc.frobeniusdist and ExplicitOpModelCalc.residuals. Second, gauge optimization w.r.t. trace distance or infidelity can incorporate instruments just by working with operation dicts from ``target_model._excalc()`` and ``mdl._excalc()``, rather than the operation dicts in ``target_model`` and ``mdl``. This PR's solution is somewhat suboptimal in that it increases reliance on ExplicitOpModelCalc, and a private method for constructing an ExplicitOpModelCalc at that. In a subsequent PR on general leakage modeling I'll revise _create_objective_fn to replace the current solution with something more elegant.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The automatic
develop <- bugfixmerge from Corey's PR yesterday (#674) failed.The current state of bugfix includes changes from #674 and the PR I just merged, #672. There's no way that the automatic
develop <- bugfixmerge will work since it includes the changes from yesterday that failed the auto-merge. So this PR catches develop up with collective changes in #672 and #674.Tests for the candidate
develop <- bugfixmerge workflow passed (all that failed was the auto-merge), so I'm going to merge this PR without waiting for tests to pass.Notes:
It's not clear to me why the auto-merge for #674 failed. It might have something to do with the weird commit history I call out in the screenshot below.