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

[bug] DGNConv fails when no directional aggregators is used #4520

Closed
rudongyu opened this issue Sep 6, 2022 · 3 comments
Closed

[bug] DGNConv fails when no directional aggregators is used #4520

rudongyu opened this issue Sep 6, 2022 · 3 comments
Assignees

Comments

@rudongyu
Copy link
Collaborator

rudongyu commented Sep 6, 2022

🐛 Bug

DGNConv is not working when no directional aggregators is given. The message function of DGNConvTower tries to retrieve eigen vector node features that do not exist.

To Reproduce

import dgl
import torch as th
from dgl.nn import DGNConv
g = dgl.graph(([0,1,2,3,2,5], [1,2,3,4,0,3]))
feat = th.ones(6, 10)
conv = DGNConv(10, 10, ['sum'], ['identity'], 2.5)
ret = conv(g, feat)

Expected behavior

The message function only passes other related node features when no directional aggregators is given.

Environment

  • DGL 0.9.0:
  • PyTorch 1.12.0+cu113
@rudongyu rudongyu added the bug:confirmed Something isn't working label Sep 6, 2022
@rudongyu rudongyu self-assigned this Sep 6, 2022
@mufeili
Copy link
Member

mufeili commented Sep 7, 2022

Do we expect this behavior? Perhaps in that case users should use PNAConv instead?

@rudongyu
Copy link
Collaborator Author

rudongyu commented Sep 7, 2022

Do we expect this behavior? Perhaps in that case users should use PNAConv instead?

We allow eig_vec to be None in its forward method. It's necessary to check it before message passing. Plus, it could be a redundant feature for use cases like ablation study on DGNConv. What do you think?

@mufeili
Copy link
Member

mufeili commented Sep 8, 2022

  1. If the authors proposed a model architecture without eig_vec, then we just need to implement that.
  2. If the extension to the case of None for eig_vec is extremely natural, then we can extend it ourselves.
  3. Otherwise, we can make this a non-optional argument.

@frozenbugs frozenbugs removed the bug:confirmed Something isn't working label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

3 participants