Skip to content

Handle time index with exclude featurizers#3691

Merged
thehomebrewnerd merged 22 commits intomainfrom
time-index-handling
Sep 9, 2022
Merged

Handle time index with exclude featurizers#3691
thehomebrewnerd merged 22 commits intomainfrom
time-index-handling

Conversation

@thehomebrewnerd
Copy link
Contributor

Various updates to handle dropping time index properly when DateTimeFeaturizer and TimeSeriesFeaturizer are excluded for time series problems. These updates retain time index column for time series native estimators, but drop it otherwise.

@thehomebrewnerd thehomebrewnerd marked this pull request as draft September 2, 2022 15:35
@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #3691 (4d079fd) into main (4445f12) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3691     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        339     339             
  Lines      34251   34307     +56     
=======================================
+ Hits       34122   34179     +57     
+ Misses       129     128      -1     
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.5% <100.0%> (+0.2%) ⬆️
.../pipelines/time_series_classification_pipelines.py 100.0% <100.0%> (ø)
evalml/pipelines/time_series_pipeline_base.py 100.0% <100.0%> (ø)
...valml/pipelines/time_series_regression_pipeline.py 100.0% <100.0%> (ø)
evalml/tests/automl_tests/test_automl.py 99.5% <100.0%> (+0.1%) ⬆️
evalml/tests/conftest.py 98.0% <100.0%> (ø)
.../integration_tests/test_time_series_integration.py 100.0% <100.0%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@thehomebrewnerd thehomebrewnerd marked this pull request as ready for review September 6, 2022 19:39
Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thehomebrewnerd thehomebrewnerd enabled auto-merge (squash) September 7, 2022 19:27
Comment on lines +205 to +209
automl.search()

rankings = automl.rankings
for score in rankings["validation_score"].values:
assert pd.notnull(score)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test, either here or as an independent test, that should_drop_time_index is getting set correctly for all pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eccabay I added the explicit check for the value of should_drop_time_index to this test. Let me know if there is a better way to do this as I'm not super familiar with the best way to iterate overall the pipelines generated by .search().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! I don't think we have a super clean way to iterate through all the pipelines right now, so imo whatever works goes.

@thehomebrewnerd thehomebrewnerd merged commit e93f513 into main Sep 9, 2022
@thehomebrewnerd thehomebrewnerd deleted the time-index-handling branch September 9, 2022 14:35
@chukarsten chukarsten mentioned this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants