Fix mapped measurement keys of repeat_until fields in CircuitOperations #6881
Conversation
Create a _mapped_repeat_until cached property that ensures repeat_until conditions have the path and key mappings of the subcircuit applied during execution time or when calculating control_keys.
Conditions don't support params, as any unresolved symbols are expected to be measurement keys. Perhaps a future feature to consider, but outside the scope of this PR.
Built-in conditions don't support param resolvers, but custom ones plausibly could. May as well keep the functionality, and add a dummy test for coverage.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6881 +/- ##
=======================================
Coverage 97.89% 97.89%
=======================================
Files 1085 1085
Lines 95050 95098 +48
=======================================
+ Hits 93048 93095 +47
- Misses 2002 2003 +1 ☔ View full report in Codecov by Sentry. |
|
@mhucka I think another glitch. Notebook test seems to have failed to launch. |
|
Thanks for flagging this. I have a PR for the flaky pytest runs now, and will turn attention to this notebook one next. |
|
@vtomole did you want to give this a look? It's a fix for issue #6446 that you filed, and uses the circuit from that issue as a test case. (The code to create the circuit is slightly different because |
|
@daxfohl Sorry for how long I took to get back to this. In regards to #6881 (comment), I've been searching for the log where the notebook error shows up, but haven't been able to find it. It might have been replaced by GitHub when the checks were re-run manually (which I probably did trigger, as I have a habit of re-running failing tests to make sure the failure is consistent). If you didn't happen to save it or a link to it, then I think for the time being, we'll have to wait to see if it occurs again. It was unusual and I don't thik it's happened since – it might have been a glitch on the GitHub side and not indicative of a flaky test. |
There was a problem hiding this comment.
I can't comment on the technical (in terms of circuit operations) correctness, the code looks reasonable.
It's up to you, but I would find the addition of some comments helpful, e.g., to explain that the caching is not for speed (which people might assume is the reason), but rather to fix a bug in the scoping of values used in repeat_until.
|
Thanks! Good point, I'll add a couple code comments before merging. |
…ns (quantumlib#6881) * Fix repeat_until scoped keys Create a _mapped_repeat_until cached property that ensures repeat_until conditions have the path and key mappings of the subcircuit applied during execution time or when calculating control_keys. * Remove param resolver support from repeat_until Conditions don't support params, as any unresolved symbols are expected to be measurement keys. Perhaps a future feature to consider, but outside the scope of this PR. * Undo previous change, just add dummy test for resolve_parameters Built-in conditions don't support param resolvers, but custom ones plausibly could. May as well keep the functionality, and add a dummy test for coverage. * Improve tests * coverage * fix test after merge from "use_repetition_ids" changes * check for symbols in repeat_until * doc
Create a
_mapped_repeat_untilcached property ofCircuitOperationthat ensuresrepeat_untilconditions have the path and key mappings of the subcircuit applied during execution time or when calculatingcontrol_keys.Fixes #6446