fix(transforms): apply MovingAverage multiplier to the output#1004
Merged
Conversation
Collaborator
Author
Automated reviewVerdict: Approve. The fix correctly implements the documented contract Correctness (verified by source-tracing)
State / numerics
Compatibility
Conventions / minimality
No blocking issues. One non-blocking note already acknowledged in the PR description: assertions are source-traced, not device-executed. |
The documented contract is y = multiplier * mean, but set() applied the multiplier to incremental deltas, so it cancelled for a steady input and skipped the seeded first sample. Maintain a running sum and output multiplier * sum / sample_size, matching the formula for every sample. Closes #1003. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3695d17 to
79688a7
Compare
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.
Bug
MovingAverage's documented contract isy = multiplier * (1/n) * Σx("Moving average will be multiplied by multiplier before it is output"). The implementation didn't honor it:output_ = input);±multiplier * value / n), so for a steady input the multiplier cancelled entirely and only affected the transient.multiplier = 1(the default) behaved correctly as a plain moving average, but anymultiplier ≠ 1— e.g. thescaleused in the fuel-level and milone-level examples — produced wrong output.Fix
Maintain a running
sum_of the buffered samples and computeoutput_ = multiplier_ * sum_ / sample_size_on every sample, including the seeded first one. This matches the documented formula exactly.Tests
New
test/system/test_moving_average_multiplier/:multiplier × windowed meanacross several samples;multiplier = 1is still a plain moving average.Compile-verified locally with
pio test --without-uploading --without-testing -e arduino_esp32(passes). As with the rest of the suite, CI compiles tests but does not execute them; assertions were traced against the new logic and should be run on a device to confirm.Closes #1003.