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

Seasonality modularization #1141

Merged
merged 6 commits into from
Feb 8, 2023
Merged

Seasonality modularization #1141

merged 6 commits into from
Feb 8, 2023

Conversation

karl-richter
Copy link
Collaborator

@karl-richter karl-richter commented Jan 31, 2023

🔬 Background

  • Following the example of trend, the seasonality component is modularized.

🔮 Key changes

  • Created a new Seasonality abstract and FourierSeasonality class.
  • Find the new class diagram here.

📋 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.

@github-actions
Copy link

github-actions bot commented Jan 31, 2023

Model Benchmark

Benchmark Metric main current diff
PeytonManning MAE_val 0.64636 0.64636 -0.0%
PeytonManning RMSE_val 0.79276 0.79276 -0.0%
PeytonManning Loss_val 0.01494 0.01494 -0.0%
PeytonManning MAE 0.42701 0.42701 0.0%
PeytonManning RMSE 0.57032 0.57032 -0.0%
PeytonManning Loss 0.00635 0.00635 0.0%
PeytonManning time 15.8239 20.6 30.18%
YosemiteTemps MAE_val 1.72949 1.72948 -0.0%
YosemiteTemps RMSE_val 2.27386 2.27386 -0.0%
YosemiteTemps Loss_val 0.00096 0.00096 -0.0%
YosemiteTemps MAE 1.45189 1.45189 -0.0%
YosemiteTemps RMSE 2.16631 2.16631 -0.0%
YosemiteTemps Loss 0.00066 0.00066 0.0%
YosemiteTemps time 124.291 169.76 36.58%
AirPassengers MAE_val 15.4077 15.4077 0.0%
AirPassengers RMSE_val 19.5099 19.5099 0.0%
AirPassengers Loss_val 0.00196 0.00196 0.0%
AirPassengers MAE 9.86947 9.86947 -0.0%
AirPassengers RMSE 11.7222 11.7222 -0.0%
AirPassengers Loss 0.00057 0.00057 -0.0%
AirPassengers time 5.48767 7.38 34.48%
Model training plots

Model Training

PeytonManning

YosemiteTemps

AirPassengers

@alfonsogarciadecorral
Copy link
Collaborator

Hey @karl-richter
I'd suggest to follow the trend structure and creating Local/Global subclasses from FourierSeasonality.
It might not be that useful now, but it might be once we add Glocal.

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Merging #1141 (e755683) into main (55c23bb) will increase coverage by 0.06%.
The diff coverage is 93.65%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1141      +/-   ##
==========================================
+ Coverage   90.17%   90.24%   +0.06%     
==========================================
  Files          30       33       +3     
  Lines        4966     5000      +34     
==========================================
+ Hits         4478     4512      +34     
  Misses        488      488              
Impacted Files Coverage Δ
neuralprophet/forecaster.py 87.90% <ø> (ø)
neuralprophet/hdays.py 99.73% <ø> (ø)
neuralprophet/plot_forecast_plotly.py 87.72% <ø> (ø)
neuralprophet/plot_model_parameters_plotly.py 94.89% <ø> (ø)
neuralprophet/time_net.py 90.16% <77.77%> (+0.05%) ⬆️
neuralprophet/components/router.py 87.50% <87.50%> (ø)
neuralprophet/components/seasonality/fourier.py 97.14% <97.14%> (ø)
neuralprophet/components/seasonality/__init__.py 100.00% <100.00%> (ø)
...euralprophet/components/seasonality/seasonality.py 100.00% <100.00%> (ø)
neuralprophet/plot_utils.py 89.82% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@karl-richter karl-richter added the status: needs review PR needs to be reviewed by Reviewer(s) label Feb 8, 2023
@karl-richter
Copy link
Collaborator Author

karl-richter commented Feb 8, 2023

Hey @karl-richter I'd suggest to follow the trend structure and creating Local/Global subclasses from FourierSeasonality. It might not be that useful now, but it might be once we add Glocal.

Good call @alfonsogarciadecorral , I adressed your comment and create a Global and Local class of Seasonality.

Comment on lines +24 to +30
self.season_params = nn.ParameterDict(
{
# dimensions - [no. of quantiles, num_seasonalities_modelled, no. of fourier terms for each seasonality]
name: init_parameter(dims=[len(self.quantiles)] + [self.num_seasonalities_modelled] + [dim])
for name, dim in self.season_dims.items()
}
)
Copy link
Owner

Choose a reason for hiding this comment

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

In a future PR, we may want to have a seasonality object per period instantiated - thus a class should not need know the num_seasonalities_modelled.

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.

Great work!

@ourownstory ourownstory merged commit 75ccf1a into main Feb 8, 2023
@ourownstory ourownstory deleted the feature/modular_seasonality branch February 8, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review PR needs to be reviewed by Reviewer(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants