aggregators: Don't crash interpolating across gaps in the data#324
Merged
Conversation
tohojo
requested changes
Jun 9, 2026
tohojo
left a comment
Owner
There was a problem hiding this comment.
Thank you for the patch!
The fix looks good, but the test is not actually run as part of the test suite. Please add it to the top-level tests; something like this (on top of your patch):
diff --git i/unittests/__init__.py w/unittests/__init__.py
index 043569ccca37..5b2c97052d14 100644
--- i/unittests/__init__.py
+++ w/unittests/__init__.py
@@ -24,6 +24,7 @@ from __future__ import absolute_import, division, print_function, unicode_litera
import os
import unittest
from . import test_util
+from . import test_aggregators
from . import test_formatters
from . import test_metadata
from . import test_parsers
@@ -33,6 +34,7 @@ from . import test_gui
test_suite = unittest.TestSuite([test_util.test_suite,
+ test_aggregators.test_suite,
test_formatters.test_suite,
test_metadata.test_suite,
test_parsers.test_suite,
diff --git i/unittests/test_aggregators.py w/unittests/test_aggregators.py
index 8d21c75c4804..84cf927595b9 100644
--- i/unittests/test_aggregators.py
+++ w/unittests/test_aggregators.py
@@ -66,6 +66,7 @@ class TestTimeseriesAggregator(unittest.TestCase):
self.assertTrue(any(v is not None for v in data),
"valid data points should still be interpolated")
+test_suite = unittest.TestLoader().loadTestsFromTestCase(TestTimeseriesAggregator)
if __name__ == "__main__":
unittest.main()The time series aggregator builds synthetic series (sums/averages) by linearly interpolating each component series onto the common step grid. When a series contains a None value -- a gap, as produced by sparse netperf output at low rates combined with latency (see issue tohojo#265) -- and that None ends up as an interpolation anchor, the interpolation computes dv_dt = (v_next - v_prev) / (t_next - t_prev) and raises "TypeError: unsupported operand type(s) for -: 'NoneType' and 'float'", aborting the whole run before any data file is written. This triggers reliably on real saturating tests over rate-limited / high-latency paths (e.g. an RRUL run through an SQM-shaped Starlink uplink). The code already guards against t_prev being None (start/end of a series); guard the anchor values too and leave a gap (None) in the synthetic series when either is missing -- the same thing already done when the interpolation distance is too long -- instead of crashing. Add a regression test reproducing the crash with a minimal series. Signed-off-by: ooonea <[email protected]>
9bfa3d9 to
83764da
Compare
tohojo
approved these changes
Jun 9, 2026
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.
Problem
The time series aggregator builds the synthetic "totals"/"avg" series by linearly interpolating each component series onto the common step grid. When a series contains a
Nonevalue — a gap, as produced by sparse netperf output at low rates combined with latency — and thatNoneends up as an interpolation anchor, it computesdv_dt = (v_next - v_prev) / (t_next - t_prev)and raises:aborting the whole run in
postprocess(aggregate(...))before any data file is written, so the result is lost.This is the low-throughput-plus-latency scenario from #265 (which reports the resulting gaps); here a gap used as an interpolation anchor crashes the run outright. I hit it reliably running RRUL from a router through an SQM-shaped Starlink uplink (~15 Mbit/s shaped upload + ~60 ms RTT → sparse
TCP_STREAMsamples). It does not reproduce on a clean loopback run (no gaps), which is why it can go unnoticed.Fix
The code already guards
t_prev is None(start/end of a series). This guards the anchor values too: whenv_prevorv_nextisNone, leave a gap (None) — the same thing already done when the interpolation distance is too long — instead of crashing. The valid-data path is unchanged.Test
unittests/test_aggregators.pydrivesTimeseriesAggregator.aggregate()with a minimal series containing aNonegap: it raises theTypeErroron currentmainand passes with this change.Note
This fixes the crash; the gaps discussed in #265 remain (interpolating across very sparse data is not well-defined, as you noted there). Happy to adjust the approach.