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

Add TimeSeriesRegularizer #3376

Merged
merged 48 commits into from
Apr 4, 2022
Merged

Add TimeSeriesRegularizer #3376

merged 48 commits into from
Apr 4, 2022

Conversation

ParthivNaresh
Copy link
Contributor

Fixes: #3369

@ParthivNaresh ParthivNaresh changed the title Ts freq components Add TimeSeriesRegularizer Mar 15, 2022
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #3376 (5434bd6) into main (bc500dc) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #3376     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        332     334      +2     
  Lines      32576   32951    +375     
=======================================
+ Hits       32446   32821    +375     
  Misses       130     130             
Impacted Files Coverage Δ
evalml/pipelines/__init__.py 100.0% <ø> (ø)
evalml/pipelines/components/__init__.py 100.0% <ø> (ø)
...alml/pipelines/components/transformers/__init__.py 100.0% <ø> (ø)
evalml/tests/component_tests/test_utils.py 95.9% <ø> (ø)
.../components/transformers/preprocessing/__init__.py 100.0% <100.0%> (ø)
...nsformers/preprocessing/time_series_regularizer.py 100.0% <100.0%> (ø)
evalml/tests/component_tests/test_components.py 99.3% <100.0%> (ø)
...ts/component_tests/test_time_series_regularizer.py 100.0% <100.0%> (ø)
evalml/tests/conftest.py 97.5% <100.0%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc500dc...5434bd6. Read the comment docs.

@ParthivNaresh ParthivNaresh marked this pull request as ready for review March 30, 2022 15:25
@ParthivNaresh
Copy link
Contributor Author

ParthivNaresh commented Mar 31, 2022

@chukarsten @eccabay Thanks Becca! Yep pipeline and documentation changes are coming up in another PR

@ParthivNaresh
Copy link
Contributor Author

ParthivNaresh commented Mar 31, 2022

@chukarsten After adding time_index as a required param, I saw how many of our tests failed for checking importable subclasses etc because cls() would throw an error when reaching this component due to time_index not being passed. Instead of creating a separate if/else for all these tests and modifying the default_parameters class method, it might be better to leave this component to follow the design of other time series related ones. Let me know what you think?

@chukarsten
Copy link
Contributor

@chukarsten After adding time_index as a required param, I saw how many of our tests failed for checking importable subclasses etc because cls() would throw an error when reaching this component due to time_index not being passed. Instead of creating a separate if/else for all these tests and modifying the default_parameters class method, it might be better to leave this component to follow the design of other time series related ones. Let me know what you think?

That's fine, man.

Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

Let's go! Thanks for addressing changes!

@ParthivNaresh ParthivNaresh merged commit 40b7e34 into main Apr 4, 2022
@chukarsten chukarsten mentioned this pull request Apr 12, 2022
@freddyaboulton freddyaboulton deleted the ts_freq_components branch May 13, 2022 14:49
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.

Add a TimeSeriesRegularizer Component
4 participants