Repository navigation
Add shared endpoint-aware momentum input and precision diagnostics - #127
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33a28d1003
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟢 Approval recommended
The review records no active issues requiring resolution for this change.
No open issues 🎉
What changed in this PR
Adds a reusable, endpoint-aware momentum input layer for the XXX thermodynamic spectrum and moments frontends. It preserves logarithmic coordinates and symbolic endpoints, supports precision-aware scalar conversion, and records conversion diagnostics and metadata.
Changes:
- Introduces
MomentumInput,MomentumCoordinate, endpoint handling, reflection, checked scalar conversion, and precision diagnostics. - Adds mutually exclusive
--momentum,--momentum-from-pi, and--momentum-betaoptions to both thermodynamic applications. - Adds unit, CLI, frontend, export-preservation, precision, and documentation coverage.
Review note: include/bethe/momentum_input.hpp:255 contains a moderate issue: the recovery message for a decimal rounding to the 2*pi endpoint incorrectly suggests specifying pi, which changes the requested endpoint.
| File | Description |
|---|---|
tests/test_momentum_input.cpp |
Tests coordinate behavior, endpoints, reflection, precision, validation, and diagnostics. |
tests/test_cli_momentum_input.cpp |
Tests CLI exclusivity, input preservation, scalar collapse, and warnings. |
tests/CMakeLists.txt |
Registers the new unit, CLI, and frontend tests. |
tests/check_momentum_input_cli.py |
Exercises both production frontends and export behavior. |
include/bethe/momentum_input.hpp |
Implements shared endpoint-aware momentum parsing and conversion diagnostics. |
docs/xxx-structure-factor-thermo.md |
Links the structure-factor guide to momentum-input documentation. |
docs/xxx-six-spinon-structure-factor.md |
Links the six-spinon guide to momentum-input documentation. |
docs/xxx-six-spinon-moments.md |
Links the six-spinon moments guide to momentum-input documentation. |
docs/momentum-input.md |
Documents supported input forms, diagnostics, and scalar-geometry limits. |
docs/command-line.md |
Links thermodynamic CLI users to the momentum-input documentation. |
apps/spinon-thermo.hpp |
Applies shared momentum conversion to selected-sector spectrum calculations. |
apps/momentum-input.hpp |
Registers shared CLI options, warnings, and metadata reporting. |
apps/bethe-xxx-structure-factor-thermo.cpp |
Integrates shared momentum inputs into the structure-factor frontend. |
apps/bethe-xxx-moments-thermo.cpp |
Integrates shared momentum inputs into thermodynamic moment calculations. |
Review effort: Lite
|
Claude review of exact head Found 3 issues: two are posted inline, and a minor one is below. What the PR does. It adds a header-only coordinate layer, What I reviewed.
Verified (x86_64 Xeon, GCC 13.3, Release, Uni20
Issues.
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
|
Addressed the reviews from Four, Codex, Claude and Copilot in e0eb329:
The added regressions reproduce the original half-angle, diagnostic, scalar-shift, endpoint-guard and ignored-option failures before their fixes. Final GCC 13 validation: 20 native checks, 22 MPFR/binary128 checks, 21 binary128-without-MPFR checks, plus the three existing selected-sector spectrum, moment and thermodynamic precision regressions, all pass. No full local suite or sanitizer rerun is claimed. Fresh CI and exact-head peer review remain required. @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ianmccul
left a comment
There was a problem hiding this comment.
Independent Four-session follow-up review of e0eb329, including the changes since 33a28d1: no blocking findings; source review sign-off for this exact head.
I reproduced the earlier half-angle underflow and scalar-reference-conditioning failures before this correction. Fresh standalone probes compiled against the corrected source now preserve the representable 3e-324/5e-324 binary64 angles and flag both the binary64 and native-long-double cancellation examples. The comparison reconstructs the returned hexadecimal scalar angles exactly and evaluates their beta errors using 150-digit arithmetic. Ordinary decimal inputs also match their direct scalar conversion in the probes.
Validation performed in all three configurations, using the existing pinned dependency builds:
- Native: 12 library tests, 4 CLI tests, and the frontend integration script pass.
- MPFR/binary128: 14 library tests, 4 CLI tests, and the frontend integration script pass.
- Binary128 without MPFR: 13 library tests, 4 CLI tests, and the frontend integration script pass.
The reviewed follow-up also covers direct ordinary-angle preservation, explicit working-endpoint rejection, the native128 cast/preset, tolerance-option validation, and the corrected 2*pi diagnostic. The existing tests cover output preservation on errors and both CLI option orders. This is a focused review and validation, not a fresh full-suite or sanitizer run.
Six retains source/CI ownership. Required CI and completed current-revision reviewer checks remain separate merge gates.
|
Claude recheck of exact head All three issues from my review of Setup. x86_64, GCC 13.3, Release, Uni20
Base 1. binary128 without MPFR. With (b), the generated 2. Scalar q. I ran
I also swept neighbouring points: Q ∈ {0.3, 1, 1.5, 1.8, 2, 2.5, 3, 3.1, 3.14, 3.1415, 3.2, 3.5, 4, 5, 6, 6.2}, compared row by row against base:
The beta and distance paths in the native build (c) give the same fp64 q as the MPFR build (a) for 60 random 3.
With Other checks.
Not checked.
Review tier: full (code or new pull request) · model: claude-opus-5-5 · effort: high |
A momentum close to pi can lose its entire endpoint distance when parsed directly into the calculation's scalar type. Add a model-independent
MomentumInput/MomentumCoordinate<Real>interface that preservesb = log(tan(q_folded/2)), exact endpoints and reflection, and exposes explicit checked scalar conversion. This is reusable by six- and eight-spinon geometry and other frontends.The XXX thermodynamic spectrum and moment apps share three mutually exclusive inputs:
--momentum,--momentum-from-pi, and--momentum-beta. MPFR-enabled builds convert the original decimal text at independently guarded precision before rounding beta to the working type; native-only builds use the widest enabled native format. Diagnostics estimate both input-to-beta and scalar-angle conversion errors, including cancellation near pi, with a configurable absolute-beta warning threshold. Exports retain the input and conversion metadata.Scalar backends preserve ordinary decimal angles independently of the rounded working beta; beta/distance inputs are converted at reference precision before narrowing. An explicit warning tolerance requires a momentum input.
Current kernels still consume scalar q: an interior coordinate that would collapse to an endpoint fails before exports are opened. This PR provides the shared coordinate/input layer; migrating integration geometry is separate. Error estimates are roundoff diagnostics, not rigorous interval bounds or kernel error estimates.
Validation with GCC 13 and pinned Uni20 ca350254:
ci-momentum-native128preset covers this configuration separately from the current GitHub CI matrix.Documentation:
docs/momentum-input.md, linked from command-line and XXX model guides.