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

DynamicNBEATs model #1191

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

DynamicNBEATs model #1191

wants to merge 10 commits into from

Conversation

tblume1992
Copy link

By default NBEATs only utilizes a polynomial basis for the trend component. This PR adds support for several different basis functions for the trend and adds a handler for these functions in a new 'DynamicNBEATs' method.

Supports several basis functions for the trend and adds a handler for these functions
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2024

CLA assistant check
All committers have signed the CLA.

@elephaint
Copy link
Contributor

elephaint commented Nov 14, 2024

Hi, thanks for your contribution, this is really cool! Two comments:

  1. Could you please follow the contributing guidelines? Specifically, we need you to build the library so that the correct python files are generated.
  2. Why not adjust NBEATs to support different basis by adding a parameter that allows to choose the basis? I'd assume that would save a lot of code.

@tblume1992
Copy link
Author

  1. hey @elephaint I was chatting with @marcopeix about this awhile back. Unfortunately I don't have time for a PR but he wanted me to be the author of it. All the code was just in a notebook I threw into the PR.

  2. You can definitely do that and that is basically what I did. 95% of this code is just the base NBEATs code. I think though that this change would alter the 'auto' model pretty significantly since we would want to search for the basis functions AND the number of functions. So we may want to think of it as a different model.

@marcopeix marcopeix self-requested a review January 27, 2025 19:57
marcopeix
marcopeix previously approved these changes Jan 27, 2025
@marcopeix marcopeix dismissed their stale review January 27, 2025 20:12

We need to add a deprecation warning for an argument of NBEATS to ensure backward compatibility

Copy link
Contributor

@elephaint elephaint left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants