Fix smoother to remove a non-invertible design matrix bug #476
Conversation
| predicted_value = signal @ coeffs | ||
| return predicted_value | ||
|
|
||
| def savgol_coeffs(self, nl, nr): | ||
| def savgol_coeffs(self, nl, nr, poly_fit_degree): |
There was a problem hiding this comment.
add docstring for the new param.
| signal_imputed[ix - self.window_length : ix] @ coeffs | ||
| signal_imputed[ix] = self.savgol_predict( | ||
| signal_imputed[ix - self.window_length : ix], | ||
| 2, |
There was a problem hiding this comment.
How big of a difference is it using a quadratic polynomial vs the previous window_length degree polynomial (assuming im reading this right)?
There was a problem hiding this comment.
Hm, now I think I should make a different PR and start a discussion there for this. This change is unrelated to the matrix bug.
There was a problem hiding this comment.
Took this change out.
There was a problem hiding this comment.
👍. the rest of the PR looks good. One thing that I like to do when fixing a bug is write out the test case that would cause the bug and code until it passes. Then just leave it in as a test case and you have some confidence that it's resolved.
There was a problem hiding this comment.
Nice idea, I'll try that.
There was a problem hiding this comment.
sounds good, will approve once a test is added
There was a problem hiding this comment.
And a few other bad input tests for the sake of code coverage (couldn't help myself :P).
dshemetov
force-pushed
the
fix_smoother_test
branch
4 times, most recently
from
b1de029 to
7a13567
Compare
dshemetov
force-pushed
the
fix_smoother_test
branch
from
7a13567 to
a5f81ba
Compare
The error arose in the case of imputation with fewer datapoints than polynomial degree. Fixes #448. I also modified the imputation to change the polynomial fit degree at that boundary edge case and to use quadratic fitting by default (this is because imputing with polynomial_fit_degree=0 does a really poor job, so shouldn't ever be used).