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

Imp/Add new model StatsForecastAutoTBATS #2611

Merged

Conversation

cnhwl
Copy link
Contributor

@cnhwl cnhwl commented Dec 3, 2024

Checklist before merging this PR:

  • Mentioned all issues that this PR fixes or addresses.
  • Summarized the updates of this PR under Summary.
  • Added an entry under Unreleased in the Changelog.

Fixes #2609.

Summary

Add new model StatsForecastAutoTBATS from nixtla.

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.19%. Comparing base (441a58a) to head (93e209b).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
darts/models/forecasting/sf_auto_tbats.py 91.17% 3 Missing ⚠️
darts/models/__init__.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
- Coverage   94.25%   94.19%   -0.07%     
==========================================
  Files         140      141       +1     
  Lines       15427    15463      +36     
==========================================
+ Hits        14541    14565      +24     
- Misses        886      898      +12     

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

@cnhwl cnhwl changed the title Add new model StatsForecastAutoTBATS Imp/Add new model StatsForecastAutoTBATS Dec 3, 2024
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 a lot for this PR @cnhwl, it looks very good and is almost ready to be merged 🚀

Before merging could you address the points from below?

  • add entry to CHANGELOG.md
  • add model to the README.md
  • add model to darts/userguide/covariates.md in this table
  • run probabilistic tests by adding the model to darts/tests/models/forecasting/test_probabilistic_models.py

darts/models/forecasting/sf_auto_tbats.py Outdated Show resolved Hide resolved
@cnhwl
Copy link
Contributor Author

cnhwl commented Dec 9, 2024

Thank you very much for your code review and guidance! @dennisbader. I'll finish the points above soon. 🤝
By the way, I submitted PR #2544 three months ago and am waiting for code review. Is there a timeline for this?

@cnhwl
Copy link
Contributor Author

cnhwl commented Dec 9, 2024

Hi! @dennisbader I'm trying to add the models to darts/tests/models/forecasting/test_probabilistic_models.py, but I'm finding that the random_state parameter doesn't exist for StatsForecastAutoTBATS, which is preventing it from passing the test_fit_predict_determinism. I would be very grateful if you could give me some help with this!

@dennisbader
Copy link
Collaborator

Thank you very much for your code review and guidance! @dennisbader. I'll finish the points above soon. 🤝 By the way, I submitted PR #2544 three months ago and am waiting for code review. Is there a timeline for this?

Hi @cnhwl, our capacity is currently a bit limited and we still need some time to get to. Sorry for that, but we will get there eventually!

@dennisbader
Copy link
Collaborator

Hi! @dennisbader I'm trying to add the models to darts/tests/models/forecasting/test_probabilistic_models.py, but I'm finding that the random_state parameter doesn't exist for StatsForecastAutoTBATS, which is preventing it from passing the test_fit_predict_determinism. I would be very grateful if you could give me some help with this!

I see, that is true :)

Actually, in my conformal prediction PR #2552, I introduce the @random_method that can be used by any model to control the randomness (see here). This will allow you to control the sampling.

Once we merge the PR (I think by end of next week), we can add this to our StatsForecastModels as well. Is it okay for you to wait until then?

Otherwise, the PR looks great and is ready to be merged :) Thanks a lot for the great work @cnhwl 🚀

@cnhwl
Copy link
Contributor Author

cnhwl commented Dec 16, 2024

Hi! @dennisbader I'm trying to add the models to darts/tests/models/forecasting/test_probabilistic_models.py, but I'm finding that the random_state parameter doesn't exist for StatsForecastAutoTBATS, which is preventing it from passing the test_fit_predict_determinism. I would be very grateful if you could give me some help with this!

I see, that is true :)

Actually, in my conformal prediction PR #2552, I introduce the @random_method that can be used by any model to control the randomness (see here). This will allow you to control the sampling.

Once we merge the PR (I think by end of next week), we can add this to our StatsForecastModels as well. Is it okay for you to wait until then?

Otherwise, the PR looks great and is ready to be merged :) Thanks a lot for the great work @cnhwl 🚀

Thanks again for your help! @dennisbader Of course! I am more than willing to wait for your PR #2552, so please let me know when that happens. 🤝

@dennisbader
Copy link
Collaborator

Hi @cnhwl, just wanted to let you know that PR #2552 (conformal prediction) has been merged now :)

@cnhwl
Copy link
Contributor Author

cnhwl commented Dec 23, 2024

Hi @dennisbader, I've resolved the code conflict and am ready to merge anytime now! 🚀

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.

Looks great now, thanks @cnhwl 🚀

I can add probabilistic support with randomness handling later for all statsforecast models.

@dennisbader dennisbader merged commit aad1440 into unit8co:master Dec 24, 2024
9 checks passed
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 AutoTBATS from nixtla
2 participants