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

Fix: estimator getter and lagged_label_name #2246

Merged
merged 25 commits into from
Mar 2, 2024
Merged

Conversation

madtoinou
Copy link
Collaborator

Fixes #2217.

Summary

  • Fixed bug in get_multioutput_estimator() where the incorrect estimator could be returned
  • Added an attribute to store the lagged label, each one corresponding to an estimator

Other Information

  • Added tests where the estimator are over-fitted on crafted examples to check if the appropriate estimator is returned by the getter.

@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.97%. Comparing base (ccd0d42) to head (ad2e32a).
Report is 1 commits behind head on master.

Files Patch % Lines
darts/models/forecasting/regression_model.py 93.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2246      +/-   ##
==========================================
+ Coverage   93.95%   93.97%   +0.01%     
==========================================
  Files         135      135              
  Lines       13501    13511      +10     
==========================================
+ Hits        12685    12697      +12     
+ Misses        816      814       -2     

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

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Thanks for this @madtoinou.
Had some minor suggestions about target lags naming, and simplifying the test.

Maybe we can extend the get get_multioutput_estimator to get_estimator that retrieves the correct estimator, regardless whether it's a multi output model or not?

darts/models/forecasting/regression_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/regression_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/regression_model.py Outdated Show resolved Hide resolved
darts/models/forecasting/regression_model.py Outdated Show resolved Hide resolved
darts/tests/models/forecasting/test_regression_models.py Outdated Show resolved Hide resolved
darts/tests/models/forecasting/test_regression_models.py Outdated Show resolved Hide resolved
darts/tests/models/forecasting/test_regression_models.py Outdated Show resolved Hide resolved
darts/tests/models/forecasting/test_regression_models.py Outdated Show resolved Hide resolved
darts/tests/models/forecasting/test_regression_models.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Great PR, thanks @madtoinou 🚀

@dennisbader dennisbader merged commit 2117bf3 into master Mar 2, 2024
7 of 9 checks passed
@dennisbader dennisbader deleted the fix/regmodel_bugs branch March 2, 2024 12:59
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.

Order of estimators for a multivariate target series with multi_models=True
3 participants