LSTMAggregation

[rusty1s]

No description provided.

[codecov]

Codecov Report

Merging #4731 (34c7406) into master (243b707) will decrease coverage by 1.88%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4731      +/-   ##
==========================================
- Coverage   84.32%   82.43%   -1.89%     
==========================================
  Files         321      322       +1     
  Lines       17215    17237      +22     
==========================================
- Hits        14516    14209     -307     
- Misses       2699     3028     +329     

Impacted Files	Coverage Δ
torch_geometric/nn/aggr/__init__.py	100.00% <100.00%> (ø)
torch_geometric/nn/aggr/base.py	93.33% <100.00%> (+0.47%)	⬆️
torch_geometric/nn/aggr/lstm.py	100.00% <100.00%> (ø)
torch_geometric/nn/models/dimenet_utils.py	0.00% <0.00%> (-75.52%)	⬇️
torch_geometric/nn/models/dimenet.py	14.51% <0.00%> (-53.00%)	⬇️
torch_geometric/nn/conv/utils/typing.py	81.25% <0.00%> (-17.50%)	⬇️
torch_geometric/nn/resolver.py	86.36% <0.00%> (-9.10%)	⬇️
torch_geometric/nn/inits.py	67.85% <0.00%> (-7.15%)	⬇️
torch_geometric/io/tu.py	93.90% <0.00%> (-2.44%)	⬇️
torch_geometric/nn/models/mlp.py	98.52% <0.00%> (-1.48%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 243b707...34c7406. Read the comment docs.

[lightaime]

Sorry, I am not familiar with LSTM. Some comments: I don't know whether using the output of the last time step is a good idea or not. 1. many padded zeros and fed into LSTM for nodes with smaller degrees. 2. the forgetting issue for long sequences.

How about something like this to remove the effects of padded zeros and average over all the time steps (not sure how it performs):

x, mask = to_dense_batch(x, index, batch_size=dim_size)
return self.reduce(self.lstm(x)[0][mask], index, ptr, dim_size, dim, reduce='mean')

[rusty1s]

I think this is a valid point. AFAIK, GraphSAGE (and any other LSTM-style aggregation procedure, e.g., Jumping Knowledge), just read out the embeddings after the last element of the sequence has been processed, so I opted to go for this solution. It might be very interesting to explore which reduction performs better here.

[lightaime]

Sure, it is worth trying it out later.