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

[major] lagged regressor with interaction modeling (shared NN) #903

Merged
merged 55 commits into from
Apr 26, 2023

Conversation

karl-richter
Copy link
Collaborator

@karl-richter karl-richter commented Oct 25, 2022

Status quo
For each covariate (aka lagged regressor), a seperate net is trained.

Change
All lagged regressors share a network. In plot_parameters, the attribution of each covariate on the forecast is derived using the Captum model attribution method for deep networks.

@karl-richter karl-richter marked this pull request as ready for review October 25, 2022 21:56
@karl-richter karl-richter marked this pull request as draft October 25, 2022 21:56
@karl-richter karl-richter self-assigned this Oct 26, 2022
@karl-richter karl-richter marked this pull request as ready for review October 26, 2022 22:56
@karl-richter karl-richter added the status: needs review PR needs to be reviewed by Reviewer(s) label Nov 3, 2022
@alfonsogarciadecorral
Copy link
Collaborator

alfonsogarciadecorral commented Nov 6, 2022

Hi @karl-richter it looks good to me in terms of model architecture changes

before:
Screenshot 2022-11-06 at 16 50 36_main

now:
Screenshot 2022-11-06 at 16 22 31_903

@ourownstory ourownstory added the priority:P1 High priority label Feb 22, 2023
Copy link
Collaborator

@alfonsogarciadecorral alfonsogarciadecorral left a comment

Choose a reason for hiding this comment

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

Hi @karl-richter @ourownstory

Nice one, I think this is a nice improvement!

A few things from me:

  • num_hidden_layers - Maybe we can rename this to num_hidden_layers_lagged_regressors or, if want to share with ar_net, num_hidden_layers_lagged. For example, Im building a shared network for future_regressors and I should be able to use a different configuration.
  • Out of curiosity, how come you use that function/formula to create d_hidden if it is not passed by user?
  • Would it make sense to pass the neural network architecture as an array? Something like covar_net_layers_array which would replace current n_hidden and num_hidden_layers

@alfonsogarciadecorral
Copy link
Collaborator

Hey @karl-richter @ourownstory

The commit I did was about removing num_hidden_layer and d_hidden and instead initialiting the ar_net and covar_net networks through then new variables: ar_net_layers_array and covar_net_layers_array

Have a look, because it implies a few changes:

  • We remove all which was related with num_hidden_layer and d_hidden . So now the user either specifies the network or it assumes num_hidden_layers=0. There is no option to pass the num_hidden_layers and then getting the d_hidden through a heuristic method.
  • config_model --> this will no longer contain num_hidden_layer and d_hidden and it will have the covar_net_layers_array
  • I removed the possibilty of passing the ‘local’ network on method add_lagged_regressor as it is a shared network now.

If you agree on the new changes there are two things missing that I will do as soon as you give thumbs up
IMPORTANT -MISSING:

  • Change tutorials documentation to remove the num_hidden_layer and d_hidden
  • Add an warn message saying that num_hidden_layer and d_hidden is not used anymore

@judussoari judussoari changed the title [feature] lagged regressor with interaction modeling (shared NN) [major] lagged regressor with interaction modeling (shared NN) Apr 21, 2023
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.

Excellent work @karl-richter and @alfonsogarciadecorral !!

@noxan noxan dismissed alfonsogarciadecorral’s stale review April 26, 2023 01:37

oskar reviewed it once more

@noxan noxan merged commit 1e31e11 into main Apr 26, 2023
@noxan noxan deleted the feature/regressors_shared_net branch April 26, 2023 01:38
@ourownstory ourownstory removed status: needs review PR needs to be reviewed by Reviewer(s) priority:P1 High priority labels Apr 26, 2023
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.

6 participants