Added GraphSAGE MaxPoolAggregator and MeanPoolAggregator and tests #278
Conversation
|
|
||
| self.has_bias = bias | ||
| self.act = activations.get(act) | ||
| self.w_neigh = None |
There was a problem hiding this comment.
self.w_neigh doesn't seem to be used
There was a problem hiding this comment.
True! I renamed what I used in build(...) and forgot to update the code in __init__.
There was a problem hiding this comment.
So now that MeanPoolAggregator is changed to match the structure of MaxPoolAggregator, this no longer applies, right?
There was a problem hiding this comment.
Yes – this no longer applies.
…as MaxPoolAggregator
…graph/stellargraph into feature/pool_aggregators
|
Addressed review comments and checked the original GraphSAGE code. The implementations of MaxPoolAggregator and MeanPoolAggregator should now match the reference code, although the reference code allows multiple Dense layers to be stacked when calculating the neighbour activations, as they mention in the paper. I don't think we should replicate this behaviour here. I have an idea to make our GraphSAGE more flexible and allow all aggregators to be implemented with less work. I'll make a new issue for it. |
…graph/stellargraph into feature/pool_aggregators
…graph/stellargraph into feature/pool_aggregators
|
FYI: When experimenting with new aggregators, you've introduced a mismatch in the aggregators used in train() and test() in |
|
Thanks for catching the aggregator mismatch! |
|
Two aggregators have been implemented:
MaxPoolAggregator (from the GraphSAGE paper)
MeanPoolAggregator (This is not quite the GCN aggregator, but similar)