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

[devops] Upgrade to lightning 2.0 [WIP] #1514

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

Conversation

ourownstory
Copy link
Owner

@ourownstory ourownstory marked this pull request as draft January 17, 2024 00:41
@ourownstory ourownstory changed the title [devops] Upgrade lightning 2 [WIP] [devops] Upgrade to lightning 2.0 [WIP] Jan 17, 2024
Copy link

github-actions bot commented Jun 20, 2024

Model Benchmark

Benchmark Metric main current diff
PeytonManning MAE_val 0.34955 0.35511 1.59%
PeytonManning RMSE_val 0.50049 0.50375 0.65%
PeytonManning Loss_val 0.01771 0.01801 1.69%
PeytonManning train_loss nan 0.01463 0.0%
PeytonManning reg_loss nan 0 0.0%
PeytonManning MAE 0.34653 0.34734 0.24%
PeytonManning RMSE 0.49312 0.49386 0.15%
PeytonManning Loss 0.01464 0.01463 -0.09%
PeytonManning time 11.4763 12.64 10.14%
AirPassengers MAE_val 30.6285 30.8786 0.82%
AirPassengers RMSE_val 31.5254 31.8365 0.99%
AirPassengers Loss_val 0.01277 0.01303 1.98%
AirPassengers train_loss nan 0.00071 0.0%
AirPassengers reg_loss nan 0 0.0%
AirPassengers MAE 6.16861 6.85845 11.18%
AirPassengers RMSE 7.85225 8.90745 13.44%
AirPassengers Loss 0.00065 0.00076 16.92%
AirPassengers time 7.39178 7.98 7.96%
EnergyPriceDaily MAE_val 5.41157 5.64156 4.25% ⚠️
EnergyPriceDaily RMSE_val 6.71538 7.19554 7.15%
EnergyPriceDaily Loss_val 0.02525 0.0289 14.47%
EnergyPriceDaily train_loss nan 0.02962 0.0%
EnergyPriceDaily reg_loss nan 0 0.0%
EnergyPriceDaily MAE 5.94936 6.40067 7.59%
EnergyPriceDaily RMSE 7.9833 8.56962 7.34%
EnergyPriceDaily Loss 0.02579 0.02941 14.05%
EnergyPriceDaily time 13.9283 15.92 14.3%
YosemiteTemps MAE_val 0.56412 0.48186 -14.58% 🎉
YosemiteTemps RMSE_val 0.83161 0.71525 -13.99% 🎉
YosemiteTemps Loss_val 0.0004 0.0003 -26.0% 🎉
YosemiteTemps train_loss nan 0.00103 0.0%
YosemiteTemps reg_loss nan 0 0.0%
YosemiteTemps MAE 0.98449 0.85851 -12.8% 🎉
YosemiteTemps RMSE 1.75389 1.54684 -11.81% 🎉
YosemiteTemps Loss 0.00132 0.00103 -21.4% 🎉
YosemiteTemps time 30.3274 35.56 17.25%
\nModel training plots\n ## Model Training ### PeytonManning ![](https://asset.cml.dev/47ad950ff09f3c3dc8330bcb4b099cb863b49822?cml=svg%2Bxml&cache-bypass=28198e1b-ded7-4836-8cd7-65104f4ba195) ### YosemiteTemps ![](https://asset.cml.dev/d90264287cf3ec56c37a78e2000605c182653cd3?cml=svg%2Bxml&cache-bypass=e3d2b5fc-2703-4740-a55a-f215140a219d) ### AirPassengers ![](https://asset.cml.dev/f3348e86540e9386558db4cfe142eb030b43c080?cml=svg%2Bxml&cache-bypass=1f5e4430-c6d5-450a-b128-92e7b87ca52a) ### EnergyPriceDaily ![](https://asset.cml.dev/815ef80a46b222e077b28bb7afb926cd2e2874a3?cml=svg%2Bxml&cache-bypass=367d3592-ffcf-4fae-86e5-0ef8c890ae8d) \n

Copy link
Owner Author

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

It seems like the tests are yielding differing metrics - Do you have any idea what might be the cause?

pyproject.toml Outdated
@@ -16,28 +16,27 @@ classifiers = [
Homepage = "https://github.com/ourownstory/neural_prophet"

[tool.poetry.dependencies]
python = "^3.9"
typing-extensions = "^4.5.0"
python = ">=3.9,<3.12"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Is there a reason that we are no longer including python 3.12?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't changed that requirement. I'll add it shortly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Please make sure that all changes to the requirements are intentional and not merge-artifacts.

@MaiBe-ctrl
Copy link
Collaborator

@ourownstory Not sure where the differences in the metrics are coming from, but I had to cast some variables to float32 to fix some tests which might caused these differences.

@ourownstory ourownstory marked this pull request as ready for review June 21, 2024 00:23
Copy link
Owner Author

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

Looks like two changes to the if-check have unintentional consequences. @MaiBe-ctrl

@@ -311,12 +311,12 @@ def __post_init__(self):
self.trend_local_reg = False

# If trend_local_reg = True
if self.trend_local_reg == True:
if self.trend_local_reg:
Copy link
Owner Author

Choose a reason for hiding this comment

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

This change looks wrong - see comment in other PR

@@ -398,12 +398,12 @@ def __post_init__(self):
self.seasonality_local_reg = False

# If seasonality_local_reg = True
if self.seasonality_local_reg == True:
if self.seasonality_local_reg:
Copy link
Owner Author

Choose a reason for hiding this comment

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

This also overwrites existing config, which should not happen

log.error("trend_local_reg = True. Default trend_local_reg value set to 1")
self.trend_local_reg = 1

# If Trend modelling is global.
if self.trend_global_local == "global" and self.trend_local_reg:
if self.trend_global_local == "global" and self.trend_local_reg is True:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Here, your change was actually correct before:
if self.trend_global_local == "global" and self.trend_local_reg:

log.error("seasonality_local_reg = True. Default seasonality_local_reg value set to 1")
self.seasonality_local_reg = 1

# If Season modelling is global.
if self.global_local == "global" and self.seasonality_local_reg:
if self.global_local == "global" and self.seasonality_local_reg is True:
Copy link
Owner Author

Choose a reason for hiding this comment

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

Here, your change was actually correct before:
if self.global_local == "global" and self.seasonality_local_reg:

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.

None yet

2 participants