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] Make tests deterministic #1600

Merged
merged 5 commits into from
Jun 26, 2024
Merged

Conversation

MaiBe-ctrl
Copy link
Collaborator

@MaiBe-ctrl MaiBe-ctrl commented Jun 26, 2024

  • Add flag to make tests deterministic to better track the metrics and performance of the fitted models.

@MaiBe-ctrl MaiBe-ctrl changed the title Make tests deterministic [Minor] Make tests deterministic Jun 26, 2024
Copy link

github-actions bot commented Jun 26, 2024

Model Benchmark

Benchmark Metric main current diff
PeytonManning MAE_val 0.35511 0.35511 0.0%
PeytonManning RMSE_val 0.50375 0.50375 0.0%
PeytonManning Loss_val 0.01801 0.01801 0.0%
PeytonManning train_loss 0.01463 0.01463 0.0%
PeytonManning reg_loss 0 0 0.0%
PeytonManning MAE 0.34734 0.34734 0.0%
PeytonManning RMSE 0.49386 0.49386 0.0%
PeytonManning Loss 0.01463 0.01463 0.0%
PeytonManning time 16.2464 14.25 -12.29% 🎉
AirPassengers MAE_val 30.8786 30.8786 0.0%
AirPassengers RMSE_val 31.8365 31.8365 0.0%
AirPassengers Loss_val 0.01303 0.01303 0.0%
AirPassengers train_loss 0.00071 0.00071 0.0%
AirPassengers reg_loss 0 0 0.0%
AirPassengers MAE 6.85845 6.85845 0.0%
AirPassengers RMSE 8.90745 8.90745 0.0%
AirPassengers Loss 0.00076 0.00076 0.0%
AirPassengers time 10.2769 8.59 -16.41% 🎉
EnergyPriceDaily MAE_val 5.64156 5.64156 0.0%
EnergyPriceDaily RMSE_val 7.19554 7.19554 0.0%
EnergyPriceDaily Loss_val 0.0289 0.0289 0.0%
EnergyPriceDaily train_loss 0.02962 0.02962 0.0%
EnergyPriceDaily reg_loss 0 0 0.0%
EnergyPriceDaily MAE 6.40067 6.40067 0.0%
EnergyPriceDaily RMSE 8.56962 8.56962 0.0%
EnergyPriceDaily Loss 0.02941 0.02941 0.0%
EnergyPriceDaily time 19.9239 17.51 -12.12% 🎉
YosemiteTemps MAE_val 0.48186 0.48186 0.0%
YosemiteTemps RMSE_val 0.71525 0.71525 0.0%
YosemiteTemps Loss_val 0.0003 0.0003 0.0%
YosemiteTemps train_loss 0.00103 0.00103 0.0%
YosemiteTemps reg_loss 0 0 0.0%
YosemiteTemps MAE 0.85851 0.85851 0.0%
YosemiteTemps RMSE 1.54684 1.54684 0.0%
YosemiteTemps Loss 0.00103 0.00103 0.0%
YosemiteTemps time 45.3692 39.31 -13.36% 🎉
\nModel training plots\n ## Model Training ### PeytonManning ![](https://asset.cml.dev/a081cefd75c9a4c8e7d9af989beb812ff1a35ee5?cml=svg%2Bxml&cache-bypass=f90bd096-a301-46a9-87d6-e9d6de94338c) ### YosemiteTemps ![](https://asset.cml.dev/3788d85efe7d00c3966556d15d0821e01af3904b?cml=svg%2Bxml&cache-bypass=e840755a-9061-49a2-a15d-2d55285ead57) ### AirPassengers ![](https://asset.cml.dev/94c42fae171cf46d7ad3e4461a1900c92ac07fa7?cml=svg%2Bxml&cache-bypass=2558c24e-1bb4-4e1f-939e-b419141da424) ### EnergyPriceDaily ![](https://asset.cml.dev/cf49507633dca610bf2298513d5de58b58cda6e8?cml=svg%2Bxml&cache-bypass=c9361ac1-5ea5-4973-af15-264e83f280eb) \n

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 to see the tests be deterministic now!

I looked through the code and have 3 thoughts:

  • What do you think about moving the deterministic flag from the NeuralProphet class init to the train method? AFAIK it only affects training?
  • Can you please update the tutorial on reproducibility to include this new flag?
  • Looks like many of the 'glocal' seasonality tests are removed - is this intentional?

@MaiBe-ctrl
Copy link
Collaborator Author

  • Yes, actually it makes more sense to have the flag in the train rather than the model itself. I will update it.
  • For some reason, some of the tests in glocal are duplicated. That's also the reason for the linting issues.

@ourownstory ourownstory added this to the 1.0.0rc10 milestone Jun 26, 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!

pyproject.toml Outdated
@@ -34,6 +34,7 @@ plotly = ">=5.13.1"
kaleido = "0.2.1" # required for plotly static image export
plotly-resampler = { version = ">=0.9.2", optional = true }
livelossplot = { version = ">=0.5.5", optional = true }
lightning-fabric = "^2.3.0"
Copy link
Owner

Choose a reason for hiding this comment

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

change to >=2.0.0

@MaiBe-ctrl MaiBe-ctrl merged commit 92a0198 into main Jun 26, 2024
10 of 11 checks passed
@MaiBe-ctrl MaiBe-ctrl deleted the bug/make_tests_deterministic branch June 26, 2024 22:10
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.

2 participants