accumulators: reject delta_N < 1 in core ctor and SI setter (bug-sweep #24)#5358
Draft
RudolfWeeber wants to merge 1 commit into
Draft
accumulators: reject delta_N < 1 in core ctor and SI setter (bug-sweep #24)#5358RudolfWeeber wants to merge 1 commit into
RudolfWeeber wants to merge 1 commit into
Conversation
…#24) The Python accumulator API documented delta_N as a positive integer sample spacing, but no layer validated it. delta_N <= 0 reached the core unchecked: - Correlator(delta_N=0): m_dt = time_step * 0 = 0, so the m_tau_max <= m_dt guard (e.g. 2.0 <= 0.0) is false; initialize_operations() then computes ceil(1 + log2(inf)) and static_cast<int>(inf) for the hierarchy depth (UB), producing a bad multi_array resize. Observed as MemoryError (std::bad_array_new_length) at construction. This UB does not throw, so the existing parallel_try_catch around construction did not catch it. - MeanVarianceCalculator/TimeSeries/ContactTimes(delta_N=0) constructed fine, then auto_update_accumulators.add(acc) + integrator.run() set frequency=0, making next_update() return 0 -> integrate(0) never advances -> infinite loop (release) / assert abort (debug). - The runtime setter acc.delta_N = 0 was equally unguarded. Fix (PREVENT invalid input, matching the existing tau_lin/tau_max validation convention): add a single chokepoint guard in the core Accumulators::AccumulatorBase ctor (all four accumulators chain to it) that throws std::runtime_error("delta_N must be >= 1") before initialize() runs, killing both the Correlator UB and the auto-update hang. Wrap the previously unwrapped MeanVarianceCalculator and TimeSeries SI ctors in parallel_try_catch so the throw is MPI-coordinated (Correlator/ContactTimes already do this). Guard the SI delta_N setter the same way so acc.delta_N = 0 is rejected and leaves the previous value intact. Extend test_correlator_exceptions and add test_delta_N_validation in accumulator_correlator.py to cover delta_N in (0, -1) at construction and via the runtime setter for Correlator, MeanVarianceCalculator and TimeSeries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
The Python accumulator API documented delta_N as a positive integer sample
spacing, but no layer validated it. delta_N <= 0 reached the core unchecked:
guard (e.g. 2.0 <= 0.0) is false; initialize_operations() then computes
ceil(1 + log2(inf)) and static_cast(inf) for the hierarchy depth (UB),
producing a bad multi_array resize. Observed as MemoryError
(std::bad_array_new_length) at construction. This UB does not throw, so the
existing parallel_try_catch around construction did not catch it.
then auto_update_accumulators.add(acc) + integrator.run() set frequency=0,
making next_update() return 0 -> integrate(0) never advances -> infinite
loop (release) / assert abort (debug).
Fix (PREVENT invalid input, matching the existing tau_lin/tau_max validation
convention): add a single chokepoint guard in the core
Accumulators::AccumulatorBase ctor (all four accumulators chain to it) that
throws std::runtime_error("delta_N must be >= 1") before initialize() runs,
killing both the Correlator UB and the auto-update hang. Wrap the previously
unwrapped MeanVarianceCalculator and TimeSeries SI ctors in
parallel_try_catch so the throw is MPI-coordinated (Correlator/ContactTimes
already do this). Guard the SI delta_N setter the same way so acc.delta_N = 0
is rejected and leaves the previous value intact.
Extend test_correlator_exceptions and add test_delta_N_validation in
accumulator_correlator.py to cover delta_N in (0, -1) at construction and via
the runtime setter for Correlator, MeanVarianceCalculator and TimeSeries.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
🤖 Generated with Claude Code