Skip to content

Fix spinon Barnes defaults with optional Uni20 term budgets - #128

Merged
ianmccul merged 2 commits into
mainfrom
fix/spinon-barnes-optional-budget
Nov 8, 2026
Merged

ianmccul merged 2 commits into
mainfrom
fix/spinon-barnes-optional-budget

Conversation

@ianmccul

@ianmccul ianmccul commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Uni20 PR #118 makes log_gamma_options::max_terms optional. An unset limit compares below an integer, so both Bethe Barnes coefficient guards reject default construction with the new dependency: native contexts throw “Barnes requires sixteen native coefficients” and MPFR contexts throw “Barnes precision exceeds coefficient budget”.

Normalize the legacy integer/new optional field to an optional budget and enforce it only when set. Explicit limits remain strict, the existing coefficient-count formulas are unchanged, and Bethe's Uni20 pin remains at ca350254.

Added focused numerical regressions for default and explicit budgets in double, long double, and MPFR at 128/256 bits, checking the independent identity G(4)=2 and end-to-end rejection of insufficient explicit caps. In MPFR, Gamma is constructed first and adds its own 64 guard bits to Bethe's working precision: it requires floor((p+128)/6)+8 coefficients versus Bethe's floor((p+64)/6)+8. Gamma therefore rejects insufficient explicit caps before Bethe's defensive guard. That assertion does not independently cover the Bethe MPFR guard; default construction is the discriminating regression for the unset-budget bug. Native explicit-cap tests exercise Bethe's own guard.

Validation at 2579ad0: all three tests fail before the fix with Uni20 697e49e7; all three pass afterward with both 697e49e7 and pinned ca350254 (GCC 13, Release, MPFR enabled). CTest discovers all three cases. Follow-up 643f6d3 changes comments only; non-comment source and whitespace were checked without rerunning numerical tests. Existing pinned production is unaffected; this is a consumer compatibility fix for newer dependency overrides.

Copilot AI lite review requested due to automatic review settings November 8, 2026 07:33
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-08T11:00:47.260790Z 643f6d3 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The review identifies no unresolved issues, and the changes preserve explicit-limit validation while covering default and supported-precision Barnes behavior with focused regressions.

No open issues 🎉

What changed in this PR

Updates Bethe’s spinon Barnes coefficient-budget handling for compatibility with Uni20’s optional max_terms, while preserving strict validation for explicitly configured limits. Adds focused numerical regressions across native and MPFR configurations.

Changes:

  • Normalize legacy integer and optional coefficient budgets before applying Barnes guards.
  • Allow default construction when no budget is set while rejecting insufficient explicit budgets.
  • Add Barnes identity and budget regression tests for native and MPFR precisions.
  • Register the new regression executable with CTest.
File Description
include/​bethe/​detail/​spinon_barnes.hpp Applies native and MPFR coefficient-budget checks only when an explicit budget is set.
tests/​test_spinon_barnes.cpp Tests default construction, Barnes identity evaluation, and sufficient or insufficient budgets across supported precisions.
tests/​CMakeLists.txt Builds and discovers the new Barnes regression test executable.

Review effort: Lite

@ian-review-bot

Copy link
Copy Markdown
Collaborator

Claude review of exact head 2579ad07409b519e7d71b6f1efe413cab0928b53

Found 1 issue, in the test rather than the fix; details in the inline comment on tests/test_spinon_barnes.cpp:46. The MPFR "insufficient explicit cap" assertion is satisfied by Uni20's own guard. Bethe's MPFR guard cannot fire for any explicit budget.

What the PR does.

  • SpinonBarnesContext in include/bethe/detail/spinon_barnes.hpp now copies log_gamma_options::max_terms into a std::optional<std::size_t>. Both coefficient guards (MPFR and native) run only when a budget is set.
  • Without this, Uni20 697e49e7 (where max_terms became optional with {} as default) makes nullopt compare below any count, so a default-constructed context throws.
  • The PR adds tests/test_spinon_barnes.cpp: a G(4)=2 identity check plus checks for default, explicit-OK and explicit-too-small budgets. It covers the native types and MPFR at 128 and 256 bits.
  • The Uni20 pin stays at ca350254.

What I reviewed.

  • The header change and the new test.
  • log_gamma_options and log_gamma_context in Uni20 ca350254 and 697e49e7: default budget, guard order, guard bits.
  • All Bethe uses of log_gamma_options/max_terms (git grep). The only log_gamma_options::max_terms reader is this header; the xxz_thermodynamic_structure_factor max_terms is a different options type.
  • Consumers: XXXSpinonDensity (via xxx_barnes_transition_rate.hpp) and the XXX thermo apps.

Verified. All runs: GCC 13.3, Release, x86_64 (80-bit long double), float128 and MPFR on, MPLAPACK off. I configured with the build recipe from REVIEW.md and -DFETCHCONTENT_SOURCE_DIR_UNI20 pointing at Uni20 ca350254 or 697e49e7.

  • bethe_spinon_barnes_tests at head: 4/4 pass with both Uni20 revisions, both directly and via ctest -R SpinonBarnes. CTest discovers 4 cases (fp64, long_double, fp128, MPFR), not the 3 the description says, because float128 is enabled here.
  • Same test compiled against the base (17656b06) header: all 4 fail on 697e49e7 ("Barnes requires sixteen native coefficients" ×3, "Barnes precision exceeds coefficient budget" ×1) and all 4 pass on ca350254. This matches the description.
  • Neighbouring inputs, using a probe that checks G(4) and G(7)=34560 against the returned bounds:
    • Native: budgets 0 and 1 are rejected by Uni20; 2 and 15 by Bethe; 16 and 17 succeed. Results are identical on both Uni20 revisions.
    • MPFR defaults at 24, 53, 113, 1000 and 2900 bits are inside the bounds on both revisions.
    • At 2960, 3000 and 6000 bits, ca350254's default 512-term cap rejects the request, while 697e49e7 now accepts it and agrees with G(4) and G(7). That is a deliberate behaviour change from Uni20's precision-aware default.
  • On 697e49e7: bethe_xxx_spinon_density_tests (24 cases), bethe-xxx-structure-factor-thermo and bethe-xxx-moments-thermo all build. The 24 gtest cases pass, and XXXSpinonDensityPrecision.cli passes (94 s). The 24 density cases also pass on ca350254.

Not checked.

  • Clang 19 or the ASan/UBSan preset.
  • The ci-wide MPLAPACK build.
  • aarch64 and MSVC, where long double differs.
  • The rest of the CTest suite and the other apps, with either Uni20 revision.
  • CI for this head: no failed-job logs were present when I reviewed.

Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high

Comment thread tests/test_spinon_barnes.cpp Resolved
@ianmccul
ianmccul merged commit 76b3388 into main Nov 8, 2026
5 checks passed
@ianmccul
ianmccul deleted the fix/spinon-barnes-optional-budget branch November 8, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants