-
Notifications
You must be signed in to change notification settings - Fork 471
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| Fix neural nets regressor shape #1589
Conversation
Model Benchmark
\n
Model training plots\n ## Model Training ### PeytonManning ![](https://asset.cml.dev/caebf60d39dce01989ee1b29f696e25f597fa393?cml=svg%2Bxml&cache-bypass=3c6af48c-7be3-433b-b9a8-565028e6d353) ### YosemiteTemps ![](https://asset.cml.dev/3486d64ca3964ba47e24df38dc6ee1abe8944845?cml=svg%2Bxml&cache-bypass=96460348-acea-4f29-a769-8efa73bab49a) ### AirPassengers ![](https://asset.cml.dev/9d9110dd64068d96c9907f315a799eae5408435b?cml=svg%2Bxml&cache-bypass=cab6e8f5-cc70-40b8-92ba-c52ac5e2505b) ### EnergyPriceDaily ![](https://asset.cml.dev/b6ec5bebee95f077fe8dc41fd9be797cda5a2b2a?cml=svg%2Bxml&cache-bypass=94d1358e-593d-4aa5-924e-e52e79b62cb2) \n |
@@ -34,7 +34,7 @@ def __init__(self, config, id_list, quantiles, n_forecasts, device, config_trend | |||
regressor_net.append(nn.Linear(d_inputs, self.d_hidden_regressors, bias=True)) | |||
d_inputs = self.d_hidden_regressors | |||
# final layer has input size d_inputs and output size equal to no. of forecasts * no. of quantiles |
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.
^ this comment may need updating.
Q: Is this NN truly applied separately to each observation and each forecast target?
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.
It's been applied for each batch layer by layer
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.
If the removal of multiplying the outputs with n_forecasts
addresses the problem, there may be more places where this needs to be addressed.
Essentially all of the files in components/future_regressors
need to be screened for this, especially any with NN, but also auxiliary functions.
It seems like these code areas lack tests - we should add those, too.
… into smaller tests
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.
Fixed shared NN and partially fixed shared nn coef, created new bug report.
🔬 Background
🔮 Key changes
📋 Review Checklist
Please make sure to follow our best practices in the Contributing guidelines.