Skip to content
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

Extend TimeSeriesImputer to handle multiple series #4291

Merged
merged 19 commits into from
Sep 5, 2023
Merged

Conversation

MichaelFu512
Copy link
Contributor

@MichaelFu512 MichaelFu512 commented Aug 29, 2023

Pull Request Description

Extend TimeSeriesImputer to handle multiple series

Closes #4292

Example picture of what this does

This is the unstacked y from an example multiseries timeseries problem.
image

The imputer now imputes on it:
image


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.rst to include this pull request by adding :pr:123.

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch coverage: 100.0% and project coverage change: +0.1% 🎉

Comparison is base (69344b2) 99.7% compared to head (41c277e) 99.7%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4291     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        355     355             
  Lines      39458   39511     +53     
=======================================
+ Hits       39338   39391     +53     
  Misses       120     120             
Files Changed Coverage Δ
evalml/pipelines/components/component_base.py 100.0% <ø> (ø)
...nents/transformers/imputers/time_series_imputer.py 100.0% <100.0%> (ø)
.../tests/component_tests/test_time_series_imputer.py 100.0% <100.0%> (ø)
...alml/tests/utils_tests/test_nullable_type_utils.py 100.0% <100.0%> (ø)
evalml/utils/nullable_type_utils.py 100.0% <100.0%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

Just some small changes, but this looks great!

evalml/pipelines/components/component_base.py Outdated Show resolved Hide resolved
evalml/pipelines/components/component_base.py Outdated Show resolved Hide resolved
Comment on lines +749 to +750
for col in y:
y[col] = np.nan
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would benefit from another test (parametrized here) where only some of the columns are NaN, but not all of them!

Copy link
Contributor

Choose a reason for hiding this comment

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

^ Sorry, should have clarified 😅 I meant a test where some of the columns are all NaN, so we drop some columns and impute or pass through others!

Copy link
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

Just a few more comments!

Comment on lines +749 to +750
for col in y:
y[col] = np.nan
Copy link
Contributor

Choose a reason for hiding this comment

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

^ Sorry, should have clarified 😅 I meant a test where some of the columns are all NaN, so we drop some columns and impute or pass through others!

evalml/tests/component_tests/test_time_series_imputer.py Outdated Show resolved Hide resolved
evalml/utils/nullable_type_utils.py Outdated Show resolved Hide resolved
evalml/utils/nullable_type_utils.py Show resolved Hide resolved
Copy link
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

Thanks!

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!

@MichaelFu512 MichaelFu512 merged commit 93e7b97 into main Sep 5, 2023
24 checks passed
@MichaelFu512 MichaelFu512 deleted the TML-8374 branch September 5, 2023 16:09
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.

Extend TimeSeriesImputer to handle multiple series
3 participants