Repository navigation
fix(movingMedian): match graphite-web alignment and NaN handling - #948
Merged
Merged
Conversation
movingMedian had its own implementation. Its output started one step too early, integer windows did not fetch earlier data, and windows with NaN values gave wrong medians. Register movingMedian in the moving package, which already had a median branch, and remove the old package and the github.com/JaderDias/movingmedian dependency. Make Windowed.Median() incremental so that the shared implementation is close to the speed of the old library on large windows. Expected test values are generated with graphite-web.
…lesFactor With returnNaNsIfStepMismatch set to false, the data returned unchanged had its timestamps moved forward by the window size. The xFilesFactor check scanned the full window for each point. Use the NaN count that Windowed already keeps.
The sorted slice moved O(window) values on each push, which was slow for large windows. Keep the non-NaN values in two indexed heaps, so that each push costs O(log window).
Keep the movingMedian package as a thin wrapper around moving, so that movingMedian still reads returnNaNsIfStepMismatch from functionsConfig.movingMedian.
npazosmendez
marked this pull request as ready for review
October 8, 2026 19:56
carrieedwards
approved these changes
Oct 19, 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.
Fixes #523 and the one-step time shift of the
movingMedianoutput.movingMedianhad its own implementation, with these bugs:from, not atfrom + step). When you combine it with an aligned series, for exampleasPercent(x, movingMedian(x, '1h')), the first point and the last point are null.windowSize - 1points were null.github.com/JaderDias/movingmediandoes not handle NaN.xFilesFactorwas ignored.The
movingpackage already had a median branch without these bugs (most fixes came from #783). ButmovingMedianwas not registered there, so the branch was never used. This PR registersmovingMedianinmoving, and removes themovingMedianpackage and thegithub.com/JaderDias/movingmediandependency.Changes to
movingon top of that:Windowed.Median()copied and sorted the window for each point. It now uses two indexed heaps that skip NaN values, so each push is O(log window).xFilesFactorcheck uses the NaN count of the window, so it does not scan the full window for each point.returnNaNsIfStepMismatchisfalse, data that is returned unchanged now keeps its timestamps. Before, they moved forward by the window size.The expected values in the new tests come from graphite-web (
movingWindowwithmedian).movingMediannow readsreturnNaNsIfStepMismatchfrom themovingfunction config. AfunctionsConfig.movingMedianentry has no effect now.A couple of unrelated fixes follow in #950
Benchmark
Benchmarks show how the new is a bit slower, but not by orders of magnitude (on the flip side, it's more memory efficient). The trade is correctness. I'm sure this can be optimized further, but it's not my intent here.