-
Notifications
You must be signed in to change notification settings - Fork 487
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
[lightning] Speed-up #973
[lightning] Speed-up #973
Conversation
Model Benchmark
|
Codecov Report
@@ Coverage Diff @@
## main #973 +/- ##
==========================================
+ Coverage 89.81% 89.83% +0.02%
==========================================
Files 18 18
Lines 4632 4645 +13
==========================================
+ Hits 4160 4173 +13
Misses 472 472
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@Kevin-Chen0 / everyone, lets please not update this branch with the main until we have the problem fixed - otherwise it becomes impossible for me to trace back what causes the performance overhead. Thanks! |
Times |
self.reg_enabled = utils.check_for_regularization( | ||
[ | ||
config_season, | ||
config_regressors, | ||
config_lagged_regressors, | ||
config_ar, | ||
config_events, | ||
config_trend, | ||
config_holidays, | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the idea to check if regularization is enabled only once (instead of calling the regularization function with each iteration and figuring out it was not active) - yet we should only have one way to check if regularization is enabled or not - not multiple ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, especially the performance improvements. There are some parts on the internals config flags of lightning I trust you on. Also one follow up issue would be to reduce our regularization checks to a single implementation again.
work in progress, resolves #972
Ready from my side, follow up changes are in #999