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

[Minor] Add function to retrieve regressor coefficients #1597

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

Conversation

Constantin343
Copy link
Contributor

🔬 Background

🔮 Key changes

  • Added a new function to TimeNet Class which helps the user retrieve the coefficients for future regressors, lagged regressors, and events

📋 Review Checklist

  • I have performed a self-review of my own code.
  • I have commented my code, added docstrings and data types to function definitions.
  • I have added pytests to check whether my feature / fix works.

Please make sure to follow our best practices in the Contributing guidelines.

@Constantin343 Constantin343 changed the title [minor] Add function to retrieve regressor coefficients [Minor] Add function to retrieve regressor coefficients Jun 23, 2024
Copy link
Owner

@ourownstory ourownstory left a comment

Choose a reason for hiding this comment

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

Thank you Constantin!

It is looking good overall!

You are headed in the right direction - I added some comments on how you can address some issues regarding the coefficient computation.

Please also add one or more tests covering your new functionality, a micro example in the docstring, and add a short feature-guide notebook demonstrating the use of your method.

neuralprophet/time_net.py Show resolved Hide resolved
neuralprophet/time_net.py Show resolved Hide resolved
neuralprophet/time_net.py Outdated Show resolved Hide resolved
@ourownstory ourownstory linked an issue Jun 25, 2024 that may be closed by this pull request
Copy link
Owner

@ourownstory ourownstory left a comment

Choose a reason for hiding this comment

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

Thank you for upgrading the coefficient computation!

I left few small comments in the review. Beyond, Do you mind adding:

  • Please also add one or more tests covering your new functionality,
  • a micro example in the docstring,
  • and add a short feature-guide notebook demonstrating the use of your method (both directly, and indirectly via the plotting methods).
  • Please also look into the existing plotting code and make sure that that code uses your code for visualizing coefficients in plots. Please also make sure that if your code yields different results (compared to the existing plotting code; likely), that those differences are intentional/meaningful (also likely).

neuralprophet/time_net.py Outdated Show resolved Hide resolved
neuralprophet/time_net.py Show resolved Hide resolved
@Constantin343
Copy link
Contributor Author

Thanks for the feedback! I wasn't aware that some of the weight calculations were already available and used in the plotting function. I adapted the functions to integrate cleaner with the existing code.
The existing plot function didn't seem to work with NN future regressors, so I am using get_future_regressor_coefficients() there now. For the other coefficient calculations I am also using some of the existing code to avoid duplicate functionality.

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.

Unable to extract LR coefficients
2 participants