Is SAGEConv really "the GraphSAGE operator"?

[andrei-rusu]

I had a look over the implementation of SAGEConv and it really doesn't look like operators from the GraphSAGE paper.
GraphSAGE proposes the concatenation of neighborhood embedding aggregations with the node embeddings. The most similar algorithm to SAGEConv in that paper (i.e. with a mean aggregator) also seems to have a different form: mean over both the neighborhood and the current node embedding, the result of this operation being ultimately fed to a transformation W.
SAGEConv, on the other hand, mean-aggregates the neighborhoods, then feeds both the latter and the node's embedding through different linear layers before finally summing them up. There also seems to be no easy way to modify the aggregation operator or the concatenation behavior (central themes in GraphSAGE) except for creating an entirely new MessagePassing subclass.

In light of these aspects, shouldn't SAGEConv documentation at least explain the differences to the actual paper? Given this is also a model of reference, I believe an implementation that more closely resembles it should be included in a graph learning framework.

[rusty1s]

We currently provide mean and max operators for GraphSAGE following along Algorithm 1 in the paper. Note that we implement concat + transformation via two separate transformations + summation, which is mathematically identical.

To me, central part of SAGEConv is the skip-connection and applying different aggregative variants. We initially implemented the mean aggregator as denoted in the paper (Eq. (2)), but as far as I understand, this is just an additional variant which drops the skip-connection completely and therefore isn't particularly useful. Other GNN frameworks implement SAGEConv exactly this way as well.