Repository navigation
Conversation
Author
|
Hi @nasa/OnAIR maintainers @asgibson 👋 I noticed this PR has been open for a little while, and the checks are currently waiting for approval to run. I’m new to contributing here, so I just wanted to give it a gentle nudge in case it slipped through. Could someone please take a look and approve the workflow so the checks can run? I’m happy to make any changes if needed. Thanks for your time! 😊 |
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 #204
test/plugins/kalman/test_kalman_plugin.py::test_Kalman__predict_smoothes_data_and_predicts_result_using_KalmanFilter_functions_as_expected_when_initial_val_is_set(line ~362) mocks theKalmanFilterand assertssmooth.call_count == 1while asserting nothing aboutpredict()'sinitial_value. So the tests lock in the dead call and can never detect that the initial value is ignored. A proper fix needs both the source change above and an updated test assertingpredictis called withinitial_value=arg_initial_val.In
plugins/kalman/kalman_plugin.py,Plugin._predict()does:simdkalman.KalmanFilteris stateless —smooth()returns a result object and has no side effects on the filter, so the first line is dead computation (~2x cost per render). Meanwhile theinitial_valbuilt in_generate_residuals()(seeding each attribute's state with its first windowed observation) is never applied to the actual prediction, which silently falls back to simdkalman's default prior. This biases every residual used for fault flagging.Verified with simdkalman 1.0.4: running
predict()immediately aftersmooth()returns bit-identical predictions to runningpredict()alone, proving the smooth call contributes nothing.Suggested fix:
Note: the existing unit test mocks the KalmanFilter and asserts
smoothwas called while never checkingpredict's kwargs, so the test suite currently enforces the buggy behavior and would need updating alongside the fix. Happy to open a PR.🐛 THE ISSUE: Dead-code Kalman smoothing — initial_value computed but never used in prediction
File:
plugins/kalman/kalman_plugin.pyFunction:
Plugin._predict()— line 166–167Current code (buggy):
I verified all of this against simdkalman 1.0.4 source + runtime experiments:
simdkalman.KalmanFilter.smooth()is a pure function. It returns a result object; it does not mutate any internal state of the filter. I proved this empirically: callingpredict()before vs. after asmooth()call gives bit-identical results (np.allclose == True).self.kf.smooth(...)line is dead computation —_generate_residuals()carefully buildsinitial_val(a(n_headers, 2, 1)array seeding each channel's state with its first frame value), passes it into_predict, and it is thrown away. The docstring even promises "Initial value for Kalman filter" as a parameter, but the prediction runs with simdkalman's default prior instead.initial_valueis honored — noise floor drifts with signal magnitude/window position, degrading the fault detector's precision right where a safety-reasoning plugin should be most trustworthy.