[NN] Refactor the Code Structure of GT #5100
Conversation
|
To trigger regression tests:
|
rudongyu
changed the title
|
This PR is quite large. Is it possible to break it into multiple smaller PRs? |
Most modifications are just movement of existing utilities for better code structure.
|
python/dgl/nn/pytorch/gt/lap_enc.py
Outdated
| ------- | ||
| >>> import dgl | ||
| >>> from dgl import LaplacianPE | ||
| >>> from dgl.nn import LapPosEncoder |
There was a problem hiding this comment.
Why did you want to break the consistency of using Laplacian for PE and PosEncoder?
There was a problem hiding this comment.
My consideration here is to keep consistency with SpatialEncoder, DegreeEncoder, etc. However, LaplacianPosEncoder seems too long. Any suggestion?
There was a problem hiding this comment.
Alternatively, you can rename LaplacianPE to LapPE. To maintain backward compatibility, consider having an alias for now and adding a deprecation warning.
python/dgl/nn/pytorch/gt/lap_enc.py
Outdated
| self.pe_encoder = nn.TransformerEncoder( | ||
| encoder_layer, num_layers=num_layer | ||
| ) | ||
| elif self.model_type == "DeepSet": |
There was a problem hiding this comment.
Is this different from SetTransformerEncoder here?
| import torch as th | ||
| import torch.nn as nn | ||
|
|
||
| from .... import to_homogeneous |
There was a problem hiding this comment.
Can we use absolute path instead of relative?
There was a problem hiding this comment.
Absolute path is indeed more recommended than relative import. Currently relative import is a convention. Maybe we need further discussion to refactor all the code.
python/dgl/nn/pytorch/gt/lap_enc.py
Outdated
| where :math:`N` is the number of nodes in the input graph, | ||
| :math:`d` is :attr:`lpe_dim`. | ||
| """ | ||
| pos_enc = th.cat( |
There was a problem hiding this comment.
what does enc in pos_enc stand for? encoding or encoder? Please avoid unconventional abbr.
There was a problem hiding this comment.
modified to pos_encoding
python/dgl/nn/pytorch/gt/lap_enc.py
Outdated
| encoder_layer = nn.TransformerEncoderLayer( | ||
| d_model=lpe_dim, nhead=n_head, batch_first=True | ||
| ) | ||
| self.pe_encoder = nn.TransformerEncoder( |
There was a problem hiding this comment.
what does pe in pe_encoder stand for?
There was a problem hiding this comment.
pe stands for positional encoding, which is a common abbr. in this context.
Description
BiasedMHA: change/ self.scalingto* self.scalingChecklist
Please feel free to remove inapplicable items for your PR.