fix(propagator): 🐛 invalidate functionals after update_initial_operator - #225
fix(propagator): 🐛 invalidate functionals after update_initial_operator#225JoshCudbyAlgo wants to merge 4 commits into
update_initial_operator#225Conversation
expectation_value_functional and expectation_value_and_gradient_functional snapshot the initial operator's coefficients. update_initial_operator re-weighted the operator in place without invalidating functionals built beforehand, so a stale functional would silently answer for coefficients the propagator no longer holds. It now bumps a re-weight epoch counter that every functional call checks, raising the same StaleFunctionalGraph error used for a rebuilt graph. Assisted-by: ClaudeCode:claude-sonnet-5
github-actions
Bot
added
documentation
|
Docs preview: https://pr-225.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
| with pytest.raises(RuntimeError, match=r"MP object has been modified"): | ||
| functional([1.0, 2.0]) | ||
|
|
||
| def test_expectation_value_functional_invalidated_after_initial_operator_update( |
There was a problem hiding this comment.
Both of the tests you added seem to tests similar behaviour. Do you think you can:
- parameterize the test?
- add more cases that will consider also Schrodinger variant?
- add similar test for PauliPropagator?
skip parameterization if putting everything under one tests would be complicated, but I'm still finding it valuable
There was a problem hiding this comment.
yes, I second Adam here. Copilot/Claude would know how to do it.
robertodr
changed the title
update_initial_operator
robertodr
left a comment
robertodr
left a comment
There was a problem hiding this comment.
Thank you @JoshCudbyAlgo LGTM, but please address Adam's comment.
Using these sentinels in the class always makes me uneasy, because they tend to hide bugs in the long run (e.g., you add one new method that should update the counter, but forget to update it)
I don't have a good idea for a better design, maybe a callback register that schedules/forces a graph update for all these methods when an invalidating action occurs 🤔
robertodr
left a comment
robertodr
left a comment
There was a problem hiding this comment.
Setting to "Request changes" so this doesn't get merged before you handle Adam's comments
|
3 participants
Summary
expectation_value_functional and expectation_value_and_gradient_functional snapshot the initial operator's coefficients. update_initial_operator re-weighted the operator in place without invalidating functionals built beforehand, so a stale functional would silently answer for coefficients the propagator no longer holds. It now bumps a re-weight epoch counter that every functional call checks, raising the same StaleFunctionalGraph error used for a rebuilt graph.
Changes
Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.