Fix naive model with last strategy for cases with trailing NaN values#1130
Fix naive model with last strategy for cases with trailing NaN values#1130mloning merged 21 commits intosktime:mainfrom
Conversation
…sp logic of mean strategy
There was a problem hiding this comment.
To me, this looks very sensible! Thanks for the change.
I think this makes the method a lot more robust.
Now, I have to admit, I wanted to review this earlier, but I had to take some time for this since the docstrings aren't too great - otherwise I would have been done with this much earlier.
May I hence kindly ask:
- in the class docstring, explain better what is meant with "last window"
- in the class docstring, explain how nans are now handled
- add docstrings to the new and old utility functions, you will do future developers a big favour...
- add docstrings to the new tests - what does this tests for, under which conditions are what errors raised?
Another point: should the tag "handles-missing-values" now be switched to True? The default is False if it's not set.
|
@Flix6x, are you still working on this? |
Little busy, but I plan to get back to this on Wednesday evening or Monday evening. Your documentation requests seem reasonable. I'd need to find out how I could set that tag, though. Is that on a class level or strategy level? If it's on a class level, I don't think the drift strategy is robust against missing values, so then it should still be |
on the class level, it should be the "worst possible value". If you want, you can do it on the object/strategy level by using |
Thanks for the hint. I did just that.
I added slightly more detail to the docstring and an inline comment. It's already pretty detailed compared to other tests. |
fkiraly
left a comment
There was a problem hiding this comment.
looks good from my side, I now understand what this is doing - I added minor comments, neither a blocker.
|
but, obviously, you also need to fix:
(can't be merged without this) |
fkiraly
left a comment
There was a problem hiding this comment.
yes, all fine - doc checks pass now.
|
Thanks @Flix6x and sorry again for the very long delay on this one - now it's merged 🎉 |
Reference Issues/PRs
Fixes #918.
What does this implement/fix? Explain your changes.
The logic for the
"last"strategy is now much more similar to the logic for the"mean"strategy. I refactored some of the logic to internal util functions so that it's clear that both strategies transform thelast_windowaccording to the seasonal periodicity in the same way, and also tile the results in the same way. The only difference is now that the"last"strategy selects the last non-NaN value from each seasonally periodic row, while the"mean"strategy computes the mean of said row.Just to note, to me it would make sense if the
"drift"strategy would use the same util functions to:I think this would be a separate issue, but just to be clear: I am not considering working on it, unless someone points me to a use case for this particular strategy.
Does your contribution introduce a new dependency? If yes, which one?
No.
What should a reviewer concentrate their feedback on?
Any other comments?
PR checklist
For all contributions
For new estimators