Repository navigation
fix(selection): CladeThompson.tau reads non-finite budgets as full and caps before overflow (#280) - #281
Conversation
…aps before overflow (#280) +inf/NaN budget_remaining now mean a full budget (tau=1), matching CostEfficient's clamp and remaining_fraction()'s no-ceiling convention; only <=0 means exhausted. The power is computed in log space and capped at max_tau before exponentiating, so a small fraction with a steep beta no longer raises OverflowError. Non-finite cool_down_beta/max_tau are rejected.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Birfy
left a comment
There was a problem hiding this comment.
Review (Closes #280)
Verdict: LGTM — posting as COMMENT because GitHub blocks self-approve on Birfy→Birfy PRs.
Correctness
All three #280 failure modes are fixed and match the issue repro on main at HEAD:
- Non-finite budget —
+inf/ NaN →tau = 1(full exploration); only<= 0/-inf→max_tau. Aligns withCostEfficient's clamp (max(0, min(1, budget)), which already maps+inf→1 and — viamin(1.0, nan)argument order — NaN→1) and withBudgetGovernor.remaining_fraction()/ defaultbudget_remaining=1.0when there is no ceiling. - Overflow — log-space
log_tau = -β · log(b)capped beforeexpavoids the Python float**OverflowErroron steep β × tiny legal fractions (1e-9, even5e-324). Schedule for in-range values unchanged (tau(0.002) ≈ 500with β=1). - Constructor —
not (isfinite(x) and …)correctly rejects NaN/±infknobs that slipped past the old< 0/< 1checks; an infinitemax_taucan no longer handrandom.betavariatean infinite α/β.
Existing test_tau_schedule expectations are preserved; cool_down_beta == 0 short-circuit still returns 1 without touching the budget.
Design / docs fit
- Small, single-purpose fix on the surface added in #279; no API shape change.
docs/selection.mddocuments the CostEfficient-aligned reading;docs/api.mdgains thetaumethod row; CHANGELOG Fixed entry is clear and issue-linked.
Tests
Locally: tests/test_selection_hgm.py — 15 passed on this branch.
Against main code with the new test file: 6 failed, 9 passed (the three new tests / parametrize cases), matching the PR claim.
Coverage is tight: non-finite budgets, overflow + select() still works under a tiny budget, and constructor rejections including max_tau < 1.
Nits (non-blocking)
- Docstring says CostEfficient “reads” NaN as full; that is true for the current clamp expression order, but it is accidental (order-dependent
min/maxwith NaN) rather than an explicitisnanbranch. CladeThompson’s explicitmath.isnanis the clearer contract — fine as written. docs/api.mdtable line fortauis one-line only; the non-finite convention lives inselection.md/ the method docstring, which is enough.
CI
Checks were still pending / in progress at review time (build + test matrix 3.9–3.13). No merge from this review.
Ship it once CI is green.
Birfy
left a comment
There was a problem hiding this comment.
Re-review at c56fabc: still LGTM (COMMENT, since self-approve is blocked). This push only merges current main (3947aee, after #262 landed). The sole conflict was CHANGELOG.md, resolved by keeping both Fixed entries, so the branch diff against main is otherwise unchanged. tests/test_selection_hgm.py: 15 passed locally. Exact-head CI (build + 3.9–3.13) is green, the PR is MERGEABLE, the Codex review on befeca9 finished with no findings, and there is no open blocking feedback.
…hompson-tau # Conflicts: # CHANGELOG.md
Birfy
left a comment
There was a problem hiding this comment.
Re-checked after merging main (924c25c, includes #272) at 42a9cd3. The only conflict was CHANGELOG.md (### Fixed): both entries are kept, the #280 CladeThompson.tau entry above the #271 runstore entry. No code conflicts. tests/test_selection_hgm.py and tests/test_selection.py pass locally on the merge commit, and CI is fully green (build + 3.9–3.13). No unresolved review threads. LGTM, still merge-ready.
Keep both CHANGELOG Fixed bullets (#280 CladeThompson.tau + main Fixed entries).
…oth CHANGELOG bullets
Stop editing CHANGELOG.md so open PRs no longer conflict on Unreleased.
Closes #280
CladeThompson.tau()(#276/#279) mishandled three input edge cases:budget_remaining:+inf(a caller's "no ceiling") and NaN returnedmax_tau, maximal exploitation from round one. They now read as a full budget (tau = 1), matchingCostEfficient's clamp andBudgetGovernor.remaining_fraction()'s "no budget means 1.0". Only<= 0(and-inf) means exhausted.b ** -betawas evaluated before the cap, and a Python float**raisesOverflowErrorinstead of returninginf(for examplecool_down_beta=40,budget_remaining=1e-9). The exponent is now computed in log space and capped atmax_taubefore exponentiating.infcool_down_betaand NaN/infmax_tauslipped past the< 0/< 1checks; an infinite cap could handrandom.betavariatean infinite tau. Both are rejected withValueErrornow.Existing
test_tau_scheduleexpectations are unchanged.docs/api.mdwas regenerated anddocs/selection.mddocuments the budget reading.Tests: 3 new tests (7 cases) in
tests/test_selection_hgm.py; 6 fail onmainand all pass here. Full local suite passed.