Add cap to seasonal period to include decomposition - #4147
Conversation
eccabay
changed the title
Codecov Report
@@ Coverage Diff @@
## main #4147 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 349 349
Lines 37780 37781 +1
=======================================
+ Hits 37663 37664 +1
Misses 117 117
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
jeremyliweishih
left a comment
jeremyliweishih
left a comment
There was a problem hiding this comment.
LGTM - lmk if you agree on the test example. I think it would be good to have coverage for this.
| rel_max_order=order, | ||
| ) | ||
| if seasonal_period is not None: | ||
| if seasonal_period is not None and seasonal_period <= 1000: |
There was a problem hiding this comment.
in the make_pipeline tests can you make sure that the decomposer is not added if the seasonal period is > 1000? I think we can just add a case here by mocking seasonal_period.
There was a problem hiding this comment.
Looking back, I realized I completely forgot about the test_make_pipeline_controls_decomposer_time_series - I added a case in there!
There was a problem hiding this comment.
Ugh, my only issue with this is the hardcoded 1000 period. Particularly the ramifications for if a user does show up with a super high frequency dataset like the one that triggered this.
There was a problem hiding this comment.
Very fair. I'll drop it over to be a constant.
4 participants
Closes #4146
Justification for this change can be found in the issue directly. I ran performance tests, there were no changes as all our test datasets have a period of <1000.