until-N in-spec draws: draw until N pass the filters, not exactly N - #47
Open
d-burg wants to merge 2 commits into
Open
until-N in-spec draws: draw until N pass the filters, not exactly N#47d-burg wants to merge 2 commits into
d-burg wants to merge 2 commits into
Conversation
GenerationConfig.n_inspec_target keeps drawing until N draws pass both postprocess filters, capped by max_total_draws (default 5x target). n_equils becomes the initial allocation. Default None = unchanged. The stopping rule counts through filtering.passes_all_filters, the same predicate the postprocess filters use, so the count the loop stops on is the count .filter() marks selected. Factored out of the two filters; the in-loop [bnd-diag] metric queries the KD-tree in the opposite direction and would have over-counted. Serial only: parallel_generate/run_shard/emit_slurm_script reject a target. RNG stream untouched when off (jBS_scales block draw unchanged, extended lazily only past n_equils) - verified bitwise on live draws. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new until-N configuration path currently relies on int(...) coercion that can silently truncate non-integer values (and accept booleans), which can change requested ensemble sizing/cap behavior without an explicit error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds an “until-N in-spec draws” generation mode so bouquet generation can keep drawing/solving until N draws pass the same postprocess filters (coil-current spec + LCFS deviation), with a configurable attempt cap and explicit serial-only enforcement. It factors the in-spec predicate into shared helpers so the stopping condition and filter() selection agree by construction.
Changes:
- Add
GenerationConfig.n_inspec_target/max_total_drawsand implement an until-N stopping rule ingenerate_bouquet, including attempt budgeting, progress/summary reporting, and cap warnings. - Factor the “in spec” logic into
bouquet.filtering(passes_*+boundary_deviation_mm) and route both the generation loop and postprocess filters through it. - Enforce serial-only behavior in parallel entry points; add solve-free tests plus documentation updates describing the new mode and guardrails.
File summaries
| File | Description |
|---|---|
| tests/test_until_n_inspec.py | Adds solve-free tests covering shared predicate semantics, config validation, RNG invariance, attempt budgeting, and parallel rejections. |
| README.md | Documents n_inspec_target and max_total_draws in the config reference. |
| docs/workflows.md | Adds a dedicated “until-N in-spec draws” section and updates workflow documentation accordingly. |
| docs/api-reference.md | Documents the new module-level predicate helpers and their relation to until-N. |
| bouquet/TokaMaker_interface.py | Implements attempt budgeting, lazy jBS_scales extension, in-loop predicate counting, and cap warning behavior. |
| bouquet/run.py | Surfaces until-N behavior via Bouquet.generate() and improves generation summary reporting for delivered-vs-requested. |
| bouquet/parallel.py | Rejects until-N configs for parallel entry points to avoid incorrect ensemble sizing. |
| bouquet/filtering.py | Introduces shared predicate helpers and reuses them in the existing postprocess filters. |
| bouquet/config.py | Adds new generation fields and validates coherence in __post_init__. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+4586
to
+4599
| # ---- Attempt budget ------------------------------------------------- | ||
| # Without a target this is exactly range(n_equils), as it always was. | ||
| # With one, n_equils is only the initial allocation and the loop runs to | ||
| # the cap, breaking early the moment the target is met. | ||
| _until_n = None if n_inspec_target is None else int(n_inspec_target) | ||
| if _until_n is None: | ||
| _max_attempts = int(n_equils) | ||
| else: | ||
| _max_attempts = (int(max_total_draws) if max_total_draws is not None | ||
| else max(int(n_equils), 5 * _until_n)) | ||
| if _max_attempts < _until_n: | ||
| raise ValueError( | ||
| f"max_total_draws={_max_attempts} is below n_inspec_target=" | ||
| f"{_until_n}; the target could never be met") |
Comment on lines
+582
to
+598
| _tgt = self.generation.n_inspec_target | ||
| _cap = self.generation.max_total_draws | ||
| if _tgt is not None: | ||
| if int(_tgt) < 1: | ||
| raise ValueError( | ||
| "generation.n_inspec_target must be >= 1 (or None to draw " | ||
| "exactly n_equils)") | ||
| if _cap is not None and int(_cap) < int(_tgt): | ||
| raise ValueError( | ||
| f"generation.max_total_draws ({int(_cap)}) is below " | ||
| f"n_inspec_target ({int(_tgt)}): the cap would stop the " | ||
| f"run before the target could ever be met") | ||
| elif _cap is not None: | ||
| raise ValueError( | ||
| "generation.max_total_draws only applies with " | ||
| "n_inspec_target set; without a target the run draws exactly " | ||
| "n_equils") |
Comment on lines
+1179
to
+1189
| # BouquetConfig validates in __post_init__, but the documented notebook | ||
| # idiom mutates fields afterwards (`bq.generation.n_equils = ...`), so | ||
| # re-check the until-N pair here -- the point where they take effect. | ||
| if gc.n_inspec_target is not None and int(gc.n_inspec_target) < 1: | ||
| raise ValueError("generation.n_inspec_target must be >= 1 or None") | ||
| if gc.n_inspec_target is None and gc.max_total_draws is not None: | ||
| import warnings as _w | ||
| _w.warn( | ||
| "max_total_draws has no effect without n_inspec_target: " | ||
| f"this run draws exactly {n_equils} (max_total_draws=" | ||
| f"{gc.max_total_draws} ignored).", UserWarning, stacklevel=2) |
…fast, executed loop coverage Adversarial-review findings on PR #47: - The cap-missed RuntimeWarning (and every [until-N] line) was swallowed by capture_native_output on the default quiet path -- a run that failed to deliver the requested ensemble returned with zero visible signal unless .filter() happened to run. The accounting block now stores a per-draw until_n_inspec verdict (plus rms/max/reasons) on the diagnostics, and Bouquet.generate re-derives the outcome OUTSIDE the capture via filtering.until_n_delivered, warning visibly on a miss. - Coil-channel fail-fast: SKIP_HARD=1 / coil_drift=None / no recon Ip NaN every verdict, which with a target set is a livelock to the attempt cap (hours of solves); it now raises pre-loop with the cause named, mirroring the boundary-channel guard. That guard's shape check now also mirrors boundary_deviation_mm's (ndim==2, not just len>=2). - The loop's arithmetic is now executed by tests, not grepped: the budget resolution, the verdict glue, and the lazy scale extension are extracted into _resolve_attempt_budget / _until_n_verdict / _extend_scale_block, closing four one-token regressions the old source-text asserts waved through (including a positional F/VSC swap invisible while both thresholds are 2%). _resolve_attempt_budget also validates the target for DIRECT generate_bouquet callers (target=0 used to 'meet' itself after one draw; bools and non-integral floats now raise) and announces an explicit cap below the allocation. - The archived [in-spec] flag routes through passes_coil_spec instead of a second inline implementation; _report_bnd documents its deliberately opposite query direction; cap<target set by post-construction mutation is caught in Bouquet.generate before the baseline work; README and workflows.md state the cap default's n_equils floor. 15 new behavioral tests (50 in the feature file); fast suite 439 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Until-N-in-spec: draw until N draws pass the filters, not exactly N
GenerationConfig.n_inspec_target = Nkeepsgenerate()drawing until Ndraws pass both postprocess filters (coil-current spec + LCFS deviation),
capped by
max_total_draws(default5 × N, never belown_equils).n_equilsbecomes the initial allocation rather than the total, so a40%-yield shot spends ~2.5× the solves of a 100%-yield shot for the same
delivered ensemble. Default
None= historical behaviour, bit-identical:the sampler's RNG stream is untouched when the feature is off (the
jBS_scalesblock draw is unchanged and only extended lazily pastn_equils; verified bitwise on live draws).One predicate, two call sites
The stopping rule counts through new
filtering.passes_all_filters— thesame
passes_coil_spec/passes_boundary_spec/boundary_deviation_mmcomposition the postprocess filters now also call — so the count the loop
stops on is the count
.filter()marksselected. This is factored out ofthe two filters rather than re-implemented: the in-loop
[bnd-diag]printqueries the KD-tree in the opposite direction and would have over-counted.
Where the in-loop verdict can diverge (a draw whose high-res LCFS trace
failed), it diverges conservatively: "no measurement → not in spec", so the
delivered ensemble is at least N selected, never fewer.
Guardrails
generate()(the notebookidiom mutates fields after construction): target ≥ 1, cap ≥ target,
cap-without-target warns.
BNDDIAG=0(no baseline contour) failsfast with the cause named, instead of grinding to the attempt cap on
all-NaN verdicts.
RuntimeWarningthat the requested ensemble was NOT delivered; thearchive holds every attempt either way.
was met, so a short bouquet cannot read as a completed run.
run_shard/parallel_generate/emit_slurm_scriptreject a config with a target set (shards cannot see each other's yield;
silently ignoring the field would hand back the wrong ensemble size).
Deliberate postprocess behavior change (flagged)
boundary_deviation_mmreturns(nan, nan)for degenerate contours(fewer than 2 points, or wrong dimensionality) where main's inline code
would build a valid 1-point KD-tree and return a real (bogus) number.
Re-running
filter_boundarieson an old archive containing such apathological contour can flip that draw's verdict pass→fail. This is the
conservative direction and intended: an unmeasurable draw is not silently
in spec.
Hardening (from adversarial review)
capture: per-draw
until_n_inspecverdicts are stored on thediagnostics and
Bouquet.generatere-derives the delivered count afterthe capture exits, warning visibly on a miss.
SKIP_HARD=1/coil_drift=NoneNaN everyverdict — with a target set that is a livelock to the attempt cap, so
it now raises pre-loop with the cause named (mirroring the boundary
guard, whose shape check now also matches the metric's).
(
_resolve_attempt_budget/_until_n_verdict/_extend_scale_block), which also validate the target for directgenerate_bouquetcallers and announce an explicit cap below theallocation. The archived
[in-spec]flag routes throughpasses_coil_spec.Tests
tests/test_until_n_inspec.py(solve-free, 50 tests): the sharedpredicate (directionality, NaN semantics, report-only mode), config
validation, the lazy
jBS_scalesextension's RNG invariance, theattempt-budget arithmetic, and the parallel rejections.
🤖 Generated with Claude Code