-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Integrate automatic period determination into AutoMLSearch #3952
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3952 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 347 347
Lines 36906 36914 +8
=======================================
+ Hits 36785 36793 +8
Misses 121 121
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Just some nits 😄
evalml/pipelines/utils.py
Outdated
# Make sure there's a seasonal period | ||
order = 3 if "Q" in freq else 5 | ||
temp_decomp = STLDecomposer(time_index, rel_max_order=order) | ||
seasonal_period = temp_decomp.determine_periodicity(X, y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're utilizing determine_periodicity
like this - maybe it should be a class method?
|
||
stl = STLDecomposer() | ||
stl.fit(X, y) | ||
assert period * 0.99 <= stl.period <= period * 1.01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment why we use 0.99
and 1.01
? Looks somewhat arbitrary unless you have the context!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming there's a margin of error with the period determination that this test accounts for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once Jeremy's comments are addressed as well!
"BAS", | ||
"BH", | ||
] | ||
invalid_frequencies = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete this line entirely if there are no invalid frequencies?
|
||
stl = STLDecomposer() | ||
stl.fit(X, y) | ||
assert period * 0.99 <= stl.period <= period * 1.01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming there's a margin of error with the period determination that this test accounts for?
) | ||
def test_unsupported_frequencies( | ||
bad_frequency, | ||
generate_seasonal_data, | ||
): | ||
"""This test exists to highlight that the underlying statsmodels STL component won't work for minute or annual frequencies.""" | ||
"""This test exists to highlight that even though the underlying statsmodels STL component won't work |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Well done. Nice type hinting!
Resolves #3985