-
Notifications
You must be signed in to change notification settings - Fork 29
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
skip connection option added #165
Conversation
@pzhanggit @jychoi-hpc |
@pzhanggit @jychoi-hpc |
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 looks good to me. As long as we solve the conflict, I think it is ok to merge.
…in GAT layer change dimension at each conv operation
@jychoi-hpc I manually solved the conflicts and rebase to maintain a linear history on the main branch. Would you mind re-controlling that I di don't remove anything of what you did? Thanks. |
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.
Look good to me too. I only have a few minor comments. Will approve once they are fixed and the tests pass.
hydragnn/models/Base.py
Outdated
@@ -100,6 +140,15 @@ def __init__( | |||
if self.initial_bias is not None: | |||
self._set_bias() | |||
|
|||
self.layers = ModuleList() | |||
for i, (conv, batch_norm) in enumerate(zip(self.convs, self.batch_norms)): |
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.
Remove i
and enumerate
since not used anymore
hydragnn/models/Base.py
Outdated
count_conv_layers = 0 | ||
|
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.
Not needed?
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.
Need to update forward
function. That skip connection is defined in self.conv_shared
but not called in forward
function. It is still calling the old self.convs
.
Update on forward
@pzhanggit @jychoi-hpc |
I found some error in my modification on ConvSequential. I will get back with a fix soon. |
@allaffa I made a fix for what I found: allaffa#4. If you merge this PR in your local repo, it will show here. It looks like unit test is mostly working for me. But, I got errors with MAE checking though:
We may need to adjust error bounds. |
Update on args
96e68a3
to
4415a11
Compare
As the name suggests, the skip connections in deep architecture bypass some of the neural network layers and feed the output of one layer as the input to the following levels. It is a standard module and provides an alternative path for the gradient with backpropagation.
Skip Connections were originally created to tackle various difficulties in various architectures and were introduced even before residual networks. In the case of residual networks or ResNets, skip connections were used to solve the degradation problems (e.g., vanishing gradient), and in the case of dense networks or DenseNets, it ensured feature reusability.
Since the residual block with the skip connection was shown to benefit the training of CNN, I thought it would be good providing it as optional also to stabilize the GNN (after all, the GNN architecture is a generalization of the CNN).