[ML] Correct ageing of windowed seasonal components #88
Conversation
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
hendrikmuhs
reviewed
May 4, 2018
| @@ -240,6 +240,7 @@ void CSeasonalComponentAdaptiveBucketing::propagateForwardsByTime(double time, b | |||
| if (time < 0.0) { | |||
| LOG_ERROR(<< "Can't propagate bucketing backwards in time"); | |||
| } else if (this->initialized()) { | |||
| time *= m_Time->fractionInWindow(); | |||
There was a problem hiding this comment.
nit: would be cleaner to simply do that on the next line without changing an input parameter
|
LGTM, only misses the changelog I think. |
|
Thanks @hendrikmuhs, I updated the change log and made your suggested change. |
|
Just to clarify, when you say "regression" in your description you mean regression w.r.t. an unreleased version? |
tveasey
force-pushed
the
bug/correct-windowed-component-ageing
branch
from
c72d88b to
ea81db2
Compare
tveasey
force-pushed
the
bug/correct-windowed-component-ageing
branch
from
ea81db2 to
ce6e71b
Compare
|
That's correct. Whilst evaluating changes related to #87, I noticed we were getting significantly worse forecasts in one of the unit tests. This turned out to be because we identified a weekday/end cyclic partition, which we didn't before. The fact that the results were worse was down to the problem this PR addresses. |
tveasey
added a commit
that referenced
this pull request
May 22, 2018
In particular, we were ageing them too fast and their decay rate should be prorated by the fraction of values with which they are updated.
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.
Investigating a forecasting regression associated with changes targeting #87 showed up an issue with the way we age periodically repeating partitions of the data (such as weekday/end splits). In particular, we were ageing them too fast and their decay rate should be prorated by the fraction of values with which they are updated. For subtle reasons, related to interaction between components in the decomposition, this could lead to biased predictions.
This can affect results for metric and count analyses when the data has distinct weekday/end repeating pattern.