Repository navigation
fix: reset the SMCABC budget, kernel and simulator on every call - #2128
aniketpandey05 wants to merge 1 commit into
Conversation
SMCABC kept simulation_counter, kernel_variance and the SASS simulator of earlier calls. A second call on the same object therefore skipped all populations after the initial one, as its budget was already used up, and kept applying the SASS transform of an earlier call, even with sass=False. Count simulations in a method instead of a closure, and reset the counter, the kernel variance and the simulator at the start of __call__.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Reset call state and verify reuse sbi/inference/abc/smcabc.py, tests/abc_test.py |
Each call resets the simulation counter and kernel variance, then uses _count_and_simulate to count parameter rows before simulation. A regression test checks that a reused instance produces the same seeded samples as a fresh instance, with or without SASS on the first call. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: janfb, manuelgloeckler, dgedon
Merge Risk: 
d3926
-
No actionable merge-blocking risk is established; the changed reuse behavior is ready for normal validation.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: resetting SMCABC state on every call, including the budget, kernel, and simulator. Description check The description follows the repository template. It explains the bug, implementation, testing, checklist status, known verification limitation, and AI assistance. The unchecked pyright item is explici… Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description follows the repository template. It explains the bug, implementation, testing, checklist status, known verification limitation, and AI assistance. The unchecked pyright item is explicitly explained, and the full suite limitation is disclosed.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2128 +/- ##
==========================================
+ Coverage 90.68% 90.70% +0.01%
==========================================
Files 142 142
Lines 14864 14867 +3
==========================================
+ Hits 13480 13485 +5
+ Misses 1384 1382 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@aniketpandey05 thanks for the PR! I will review it after November 9. Until then, please keep it concise, with as few changed lines as possible. Thanks, Jan |
What does this PR do?
SMCABCkept state from earlier calls on the same object:simulation_counterwas set to 0 only in__init__. Asecond call started with the budget of the first already used up, so it ran the
initial population and then stopped, without a warning.
sass=True, the call replacedself._simulate_with_budgetby a simulator that applies the SASS transform, and nothing restored it. Later calls
kept that transform even with
sass=False, and a second SASS call ran its pilotsimulations through the old transform.
The budget counter is now a method instead of a closure built in
__init__, and__call__resets the counter, the kernel variance and the simulator before it starts.Does this close any issues?
No issue was opened, as this is a clear bug fix.
Anything else we should know?
Same object, same call twice (
num_particles=100,num_initial_pop=500,num_simulations=3000,epsilon_decay=0.5, simulatortheta + 0.05 * noise):main, call 1main, call 2On
main, the second call returns the initial rejection-ABC population.MCABCdoes nothave this problem, because it keeps its simulator local to the call.
The new test checks a behaviour: a second call on the same object returns exactly the
samples of a fresh object with the same seed, after a first call with and without SASS.
PR #2123 also edits the start of
SMCABC.__call__, so whichever of the two is mergedsecond will need a small rebase.
Checklist
pytest -m "not slow and not gpu" tests/abc_test.pypasses (32 passed). Fullsuite not run locally.
uv run pyright sbipasses. My local environment could not resolvetorchforpyright; relying on CI.
test_smcabc_second_call_matches_a_fresh_instance(with and without SASSin the first call). Both cases fail on
mainand pass here.I used Claude Code to find this bug