Repository navigation
Gap separated training and test - #3160
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3160 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 324 324
Lines 31248 31302 +54
=======================================
+ Hits 31144 31198 +54
Misses 104 104
Continue to review full report at Codecov.
|
| @@ -15,4 +17,18 @@ def load_weather(): | |||
| + evalml.__version__ | |||
| ) | |||
| X, y = load_data(filename, index=None, target="Temp") | |||
There was a problem hiding this comment.
There were two dates missing from this dataset which prevented the new implementation in this PR from working properly. Because we no longer rely on the index to determine validity of the training/test separation, the time_index column in X has to be as we expect it to be (ordered, interval has to be detected, datetime based) for the new tools in woodwork 0.10.0 to be usable.
…ing data doesn't have valid frequency
# Conflicts: # evalml/tests/dependency_update_check/latest_dependency_versions.txt
| temporal_columns=[train_copy.ww.time_index] | ||
| ) | ||
| freq = X_frequency_dict[test_copy.ww.time_index] | ||
| if freq is None: |
There was a problem hiding this comment.
This was added regarding a conversation with @freddyaboulton. He raised the valid point that we don't want AutoMLSearch to pass only to fail when a user tries and uses predict. In this case, AutoMLSearch will raise this error if the time_index does not have values that can be inferred.
I'm up for a discussion regarding this since I know it touches a lot of time series and is essentially the first time that our whole algorithm will be directly exposed to a proper interval frequency check (outside of DateTimeFormatDataCheck).
bchen1116
left a comment
bchen1116
left a comment
There was a problem hiding this comment.
Great work here! Left some questions about test coverage that I think need to be addressed first
# Conflicts: # evalml/tests/dependency_update_check/minimum_core_requirements.txt
bchen1116
left a comment
bchen1116
left a comment
There was a problem hiding this comment.
Thanks for answering my questions! This looks good to me!
2 participants
Fixes #3078
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.