Skip to content

Fix StridedView sentinels and quadrature regressions from e941630, add ctest target - #4

Open
gouarin wants to merge 5 commits into
mainfrom
fix/strided-view-sentinels-and-quadrature-regressions
Open

Fix StridedView sentinels and quadrature regressions from e941630, add ctest target#4
gouarin wants to merge 5 commits into
mainfrom
fix/strided-view-sentinels-and-quadrature-regressions

Conversation

@gouarin

@gouarin gouarin commented Sep 7, 2026

Copy link
Copy Markdown

Summary

Compile-verified fixes for StridedView and for three regressions introduced in e941630, plus a first ctest target.

StridedView (include/LNIT/misc/StridedView*.hpp)

  • isDone() is now const. operator==(const Iterator&, Sentinel) calls it, so any comparison with the sentinel failed to compile once the view was instantiated, and std::sentinel_for / std::ranges::input_range were not satisfied. The header is not instantiated by the library build, which is why this went unnoticed.
  • The conditional_t selecting reference was inverted: the non-const iterator yielded const&.
  • Random-access advance is bounded by the distance to end() instead of stepping past it when the size is not a multiple of the stride.

Gauss-Laguerre / Gauss-Hermite

  • inner_product was given fx.end() instead of fx.begin() (3 sites), reading past the node array. This produced NaN for every moment of order >= 2 in demo_GaussLegendre and a fully-NaN Hessian in demo_LevermooreLikePDF.

AdaptiveQuadratureBase::integrate

  • const auto& [a, b] = m_intervals[maxErrIdx] aliased the entry overwritten on the next line, so b became midPoint and the second half of a split interval was integrated over an empty range. The scheme then reported convergence with a wrong value (0.163 instead of 0.5 for the first moment of N(0.5, 1)). Replaced by a copy.

Tests

  • New LNIT_BUILD_TESTS option (OFF by default) and tests/test_StridedView.cpp, framework-free, built with the library's -Werror flag set. Covers random-access, bidirectional and forward-only ranges, sizes not multiple of the stride, empty views, mutation through the non-const iterator, sentinel comparison in both operand orders, and static_asserts on the C++20 range/iterator concepts.

Verification

Toolchain: clang (zig 0.16) + libc++, aarch64, fmt 11.1.4.

  • Library + 6 demos + test build with zero warnings under the project's -Werror flags in C++20.
  • ctest: StridedView passes in C++20 and C++23. The test also passes standalone under ASan/UBSan.
  • demo_GaussLegendre: all 101 moments, all 6 tolerances, relative error <= 7e-11, no NaN (before: NaN from order 2, then wrong values after fixing only the fx.end() bug).
  • No NaN in any of the 25 demo log files; the Levermore Hessian is now a consistent Hankel-like moment matrix.
  • Not verified: full library build in C++23 mode, because ClenshawCurtisAdaptiveQuadrature_impl.hpp uses std::views::stride, which the libc++ available here does not implement yet. Unrelated to this change.

StridedView cleanup (third commit)

  • m_bound was computed from std::ranges::distance and never read. Removed. The distance call also consumed pure input ranges before iteration.
  • Single-pass ranges have move-only iterators, so Iterator's constructor now takes its arguments by value and moves them, postfix ++ returns a copy only when the inner iterator is copyable (void otherwise), and stride() forwards its argument to std::views::all.
  • New test case built on std::views::istream. Clenshaw-Curtis demo logs are bit-identical before and after this change.

Clenshaw-Curtis tables (fourth commit)

  • All five CC weight tables (9/17/33 nodes, hybrid 13, GLCC copy of 33) lacked the 1/2 factor on the last Chebyshev mode: exact only up to degree n-1 instead of n+1, and ∫T_n off by exactly a factor 2. Nodes and companion rules were correct.
  • New tools/generate_clenshaw_curtis_tables.py (mpmath, 50 digits) regenerates the tables and rewrites the headers in place; the tables now come from it.
  • New tests/test_ClenshawCurtisTables.cpp: sum of weights, symmetry, positivity, exactness for x^k up to the declared degree, ∫T_n, and failure at the next even degree. Tables are exposed to tests via a LNIT_TESTING compile definition.
  • Shared test infrastructure: tests/Check.hpp, tests/Tolerances.hpp (named tiers), CMake helper with a 30 s timeout.
  • estimateIntegralImpl lambdas declared -> LongScalar returned a Scalar: fails -Wdouble-promotion for <double, long double>, which nothing instantiated before. Explicit conversions added (6 headers).
  • Effect on demo_ClensawCurtis: max relative error over the 101 moments drops from 1.5e-8 to 7e-10 at tolerances 1e-4..1e-10. At tolerance 1e-14 it goes from 3e-12 to 5e-10 on moments of order 63 to 99 (values 1e44 to 1e79) where the error estimator is optimistic by 7 orders of magnitude. That is an estimator calibration issue, unrelated to the tables, left for a later change.

AdaptiveQuadratureBase state and bounds (fifth commit)

  • m_hasConverged was never initialized (indeterminate read on a fresh object). Default member initializers for m_hasConverged and m_it; a private resetState() also runs when an infinite-domain integration gives up on a non-finite tail.
  • integrate(f, xmin, xmax) bounds conventions: xmin > xmax returns the opposite of the integral over [xmax, xmin] (before: 0 and "converged" for a short reversed interval, about 4e9 intervals for a long one); equal bounds return 0; a NaN bound returns NaN, not converged (before: plausible value, converged); an infinite bound delegates to the infinite-domain methods.
  • New tests/test_AdaptiveQuadratureBase.cpp covering fresh-object state and every bound case for the four adaptive quadratures.

Verification of the last two commits

  • ctest: 3/3 pass in C++20; all three tests also pass standalone under ASan/UBSan.
  • Library and demos build with zero warnings under the -Werror flag set.
  • C++23: only test_StridedView could be built here, the library needs std::views::stride which the available libc++ lacks.

StridedView:
- isDone() is now const: operator==(const Iterator&, Sentinel) calls it,
  so any comparison with the sentinel failed to compile as soon as the
  view was instantiated (std::sentinel_for / input_range not satisfied).
- The conditional selecting the reference type was inverted, making the
  non-const iterator yield const references.
- In the random-access branch, the advance is now bounded by the distance
  to end() instead of stepping past it when the size is not a multiple
  of the stride.

Gauss-Laguerre / Gauss-Hermite:
- inner_product was given fx.end() instead of fx.begin(), reading past
  the node array. This produced NaN for all moments of order >= 2 in
  demo_GaussLegendre and a fully NaN Hessian in demo_LevermooreLikePDF.

AdaptiveQuadratureBase::integrate:
- `const auto& [a, b] = m_intervals[maxErrIdx]` aliased the entry that is
  overwritten on the next line, so `b` became midPoint and the second
  half of the split interval was integrated over an empty range. The
  scheme then reported convergence with a wrong value (e.g. 0.163 instead
  of 0.5 for the first moment of N(0.5, 1)). Take a copy instead.

After the fix, all 101 moments in demo_GaussLegendre have a relative
error <= 7e-11 for every tolerance, and no demo log contains NaN.
New LNIT_BUILD_TESTS option (OFF by default) enabling tests/ with a
framework-free test of StridedView: iteration over random-access,
bidirectional and forward-only ranges, sizes not multiple of the
stride, empty views, mutation through the non-const iterator, sentinel
comparison in both operand orders, and static_asserts on the C++20
range/iterator concepts. Built with the library's warning flags.
@gouarin
gouarin force-pushed the fix/strided-view-sentinels-and-quadrature-regressions branch from e594556 to f834773 Compare September 7, 2026 18:21
m_bound was computed in the constructor from std::ranges::distance and
never read. Besides being dead code, the distance call consumed pure
input ranges before the first iteration.

Removing it is not sufficient for single-pass ranges, whose iterators
are move-only, so:
- Iterator's constructor takes the inner iterator and sentinel by value
  and moves them;
- postfix ++ returns a copy only when the inner iterator is copyable,
  and void otherwise;
- stride() forwards its argument to std::views::all instead of passing
  an lvalue, which would have copied a view.

The test gains a case built on std::views::istream. Clenshaw-Curtis demo
logs are bit-identical before and after this change.
…ness

The five Clenshaw-Curtis weight tables (9/17/33 nodes, the 13-node hybrid
rule and the copy of the 33-node table in GLCC) were missing the 1/2 factor
on the last Chebyshev mode j = n/2. Each rule was therefore exact only up to
degree n-1 instead of n+1, and integrated T_n with an error of exactly a
factor 2. The nodes and the companion rules (GL15/14/6, hybrid alternate)
were correct.

tools/generate_clenshaw_curtis_tables.py recomputes the tables with mpmath
(50 digits, printed with 20) and rewrites the headers in place; the five
tables now come from that script.

tests/test_ClenshawCurtisTables.cpp checks every table: sum of weights,
symmetry, positivity, exactness for x^k up to the declared degree, the
integral of T_n, and failure at the next even degree so the declared degree
is not underestimated. The tables are exposed to the tests through a
LNIT_TESTING compile definition.

Test infrastructure: tests/Check.hpp (CHECK / CHECK_CLOSE, not compiled out
in Release) and tests/Tolerances.hpp (named tolerance tiers), shared by all
tests; tests/CMakeLists.txt gets a helper function with a 30 s timeout.

The estimateIntegralImpl lambdas declared "-> LongScalar" returned a Scalar,
which fails -Wdouble-promotion as soon as <double, long double> is
instantiated (never done by the library sources or the demos). Explicit
conversions added in the six affected headers.
…nventions

m_hasConverged was never initialized: hasConverged() on a freshly built
object read an indeterminate bool. m_hasConverged and m_it now have default
member initializers, and a private resetState() gathers the "forget the
previous integration" logic, also used when an infinite-domain integration
gives up on a non-finite tail.

integrate(f, xmin, xmax) now follows the conventions of the definite
integral instead of feeding the initial mesh a negative or NaN length:
- xmin > xmax returns -integrate(f, xmax, xmin). Before, a short reversed
  interval returned 0 and reported convergence (ceil of a small negative
  number converts to 0 intervals); a long one asked for about 4e9 intervals.
- xmin == xmax returns 0, converged.
- a NaN bound returns NaN, not converged. Before, a plausible value was
  returned and convergence reported.
- an infinite bound delegates to integrate(f), integrateLeftInfinite() or
  integrateRightInfinite().

tests/test_AdaptiveQuadratureBase.cpp covers the fresh-object state and all
bound cases for the four adaptive quadratures.
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.

1 participant