Ensure that gauge optimization incorporates instruments #672
Conversation
…tOpModelCalc.operations rather than ExplicitOpModel.operations. Replace instances of np.dot() with @ infix operator. Simplify branching in explicitcalc.py.
rileyjmurray
requested review from
coreyostrove and
kevincyoung
and removed request for
a team
|
The current fidelity-based objective tries to make fidelities as close to 1 as possible. My solution for this PR is to disable tests for fidelity-based gauge optimization in the presence of instruments. I can enable those tests as part of PR #669 after this PR is merged. |
|
During today's dev meeting @coreyostrove expressed a desire to not have our fix just rely on the ExplicitOpModelCalc class (or at least to not rely on it by calling the private function ExplicitOpModel._excalc()). While there are various ways of accomplishing this, it seems that all of them would entail significant revisions to gaugeopt.py::_create_objective_fn. This is undesirable since I have other pending PRs that affect gaugeopt.py::_create_objective_fn. The other downside to doing anything other than ExplicitOpModelCalc is that this gets tied up in how leakage is specified (e.g., with an @coreyostrove, if it's alright with you I'd prefer to merge this change in now. I can include more comprehensive changes to gaugeopt.py::_create_objective_fn in my PR that adds general leakage modeling. |
There was a problem hiding this comment.
@coreyostrove, if it's alright with you I'd prefer to merge this change in now. I can include more comprehensive changes to gaugeopt.py::_create_objective_fn in my PR that adds general leakage modeling.
PR approved conditional on you writing this ⬆️ on that todo list on your whiteboard 😉.
Great work tracking this down and patching these issues. Also glad to see we weren't actually skipping the instruments. Hopefully @pcwysoc will forgive us (me) for the false alarm!
The automatic ``develop <- bugfix`` merge from Corey's PR yesterday (#674) [failed](https://github.com/sandialabs/pyGSTi/actions/runs/18768908655/job/53549598808#step:3:20). The current state of bugfix includes changes from #674 and the PR I just merged, #672. There's no way that the automatic ``develop <- bugfix`` merge 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 <- bugfix`` merge 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. <img width="1168" height="204" alt="image" src="https://github.com/user-attachments/assets/d8f196f8-26f6-45d1-b77f-e74c63e0a0d1" />
2 participants
This resolves #644. 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()andmdl._excalc(), rather than the operation dicts intarget_modelandmdl.While I was making these changes, I (1) replaced instances of np.dot with the matmul infix operator, and (2) simplified branching in explicitcalc.py.