Fix StridedView sentinels and quadrature regressions from e941630, add ctest target - #4
Open
gouarin wants to merge 5 commits into
Open
Fix StridedView sentinels and quadrature regressions from e941630, add ctest target#4gouarin wants to merge 5 commits into
gouarin wants to merge 5 commits into
Conversation
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
force-pushed
the
fix/strided-view-sentinels-and-quadrature-regressions
branch
from
September 7, 2026 18:21
e594556 to
f834773
Compare
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.
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.
Summary
Compile-verified fixes for
StridedViewand for three regressions introduced in e941630, plus a firstctesttarget.StridedView (
include/LNIT/misc/StridedView*.hpp)isDone()is nowconst.operator==(const Iterator&, Sentinel)calls it, so any comparison with the sentinel failed to compile once the view was instantiated, andstd::sentinel_for/std::ranges::input_rangewere not satisfied. The header is not instantiated by the library build, which is why this went unnoticed.conditional_tselectingreferencewas inverted: the non-const iterator yieldedconst&.end()instead of stepping past it when the size is not a multiple of the stride.Gauss-Laguerre / Gauss-Hermite
inner_productwas givenfx.end()instead offx.begin()(3 sites), reading past the node array. This produced NaN for every moment of order >= 2 indemo_GaussLegendreand a fully-NaN Hessian indemo_LevermooreLikePDF.AdaptiveQuadratureBase::integrate
const auto& [a, b] = m_intervals[maxErrIdx]aliased the entry overwritten on the next line, sobbecamemidPointand 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
LNIT_BUILD_TESTSoption (OFF by default) andtests/test_StridedView.cpp, framework-free, built with the library's-Werrorflag 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, andstatic_asserts on the C++20 range/iterator concepts.Verification
Toolchain: clang (zig 0.16) + libc++, aarch64, fmt 11.1.4.
-Werrorflags in C++20.ctest:StridedViewpasses 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 thefx.end()bug).ClenshawCurtisAdaptiveQuadrature_impl.hppusesstd::views::stride, which the libc++ available here does not implement yet. Unrelated to this change.StridedView cleanup (third commit)
m_boundwas computed fromstd::ranges::distanceand never read. Removed. Thedistancecall also consumed pure input ranges before iteration.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), andstride()forwards its argument tostd::views::all.std::views::istream. Clenshaw-Curtis demo logs are bit-identical before and after this change.Clenshaw-Curtis tables (fourth commit)
∫T_noff by exactly a factor 2. Nodes and companion rules were correct.tools/generate_clenshaw_curtis_tables.py(mpmath, 50 digits) regenerates the tables and rewrites the headers in place; the tables now come from it.tests/test_ClenshawCurtisTables.cpp: sum of weights, symmetry, positivity, exactness forx^kup to the declared degree,∫T_n, and failure at the next even degree. Tables are exposed to tests via aLNIT_TESTINGcompile definition.tests/Check.hpp,tests/Tolerances.hpp(named tiers), CMake helper with a 30 s timeout.estimateIntegralImpllambdas declared-> LongScalarreturned aScalar: fails-Wdouble-promotionfor<double, long double>, which nothing instantiated before. Explicit conversions added (6 headers).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_hasConvergedwas never initialized (indeterminate read on a fresh object). Default member initializers form_hasConvergedandm_it; a privateresetState()also runs when an infinite-domain integration gives up on a non-finite tail.integrate(f, xmin, xmax)bounds conventions:xmin > xmaxreturns 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.tests/test_AdaptiveQuadratureBase.cppcovering fresh-object state and every bound case for the four adaptive quadratures.Verification of the last two commits
-Werrorflag set.test_StridedViewcould be built here, the library needsstd::views::stridewhich the available libc++ lacks.