-
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
[Major] Combined future regressor layer arguments into one single integer list regressors_layers
like ar_layers
#1604
Conversation
regressors_layers
like ar_layers
regressors_layers
like ar_layers
regressors_layers
like ar_layers
regressors_layers
like ar_layers
regressors_layers
like ar_layers
regressors_layers
like ar_layers
Labelled as Major as it is a breaking change. |
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.
Thank you - Looking good overall!
I left few suggestions.
neuralprophet/forecaster.py
Outdated
Number of hidden layers in the neural network model for future regressors. | ||
Ignored if ``future_regressors_model`` is ``linear``. | ||
future_regressors_layers: list of int | ||
array of hidden layer dimensions of the future regressor nets. Specifies number of hidden layers (number of entries) |
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.
to avoid confusion, phase array of..
as list of ...
Should also state default values.
Looks like we currently have [4,4]. We might want roll this back to no hidden layers by default. Thoughts?
neuralprophet/forecaster.py
Outdated
@@ -424,8 +421,7 @@ def __init__( | |||
season_global_local: np_types.SeasonGlobalLocalMode = "global", | |||
seasonality_local_reg: Optional[Union[bool, float]] = False, | |||
future_regressors_model: np_types.FutureRegressorsModel = "linear", | |||
future_regressors_d_hidden: int = 4, | |||
future_regressors_num_hidden_layers: int = 2, | |||
future_regressors_layers: Optional[list] = [4, 4], |
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.
The default for this should be identical or lower than ar_layers
and lagged_reg_layers
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.
I simply used the previous default, but agree it makes sense to set the default to []. Done that.
Model Benchmark
\n
Model training plots\n ## Model Training ### PeytonManning ![](https://asset.cml.dev/4f97574d46ef2d0c0bfed957461e33cf42e0ef85?cml=svg%2Bxml&cache-bypass=ac81b40e-35bc-414c-8b61-dc228b1e60f1) ### YosemiteTemps ![](https://asset.cml.dev/6d0e9107934a60b10516d34271843a60d26e56a5?cml=svg%2Bxml&cache-bypass=dc66f907-ebbb-4061-8e5c-32a31c4928cd) ### AirPassengers ![](https://asset.cml.dev/883586b8ccb26eeb23da09d0ccb248f42b1999b3?cml=svg%2Bxml&cache-bypass=de9eedad-d8e4-4208-9641-a164d3514bf7) ### EnergyPriceDaily ![](https://asset.cml.dev/e6a0dbda784dea6c3b0a6124cf00ea2a6732a75c?cml=svg%2Bxml&cache-bypass=64fd0eaa-6b0c-4221-9c99-194af3461bc8) \n |
🔬 Background
🔮 Key changes
Removed the arguments
future_regressors_num_hidden_layers
andfuture_regressors_d_hidden
and replaced them with a single argumentfuture_regressors_layers : Optional[List[int]]
. (following the practice ofar_layers
)📋 Review Checklist