Merge Cluster-GCN and GCN graph classification into GCN layer #1205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Check out this pull request on You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB. |
This was referenced Apr 6, 2020
huonw
added a commit
that referenced
this pull request
Apr 7, 2020
The core layer in a full batch method (like `GraphConvolution` inside `GCN`) natively computes information about a whole graph, but this doesn't directly work with machine learning (and, in particular, how it works with `Keras`). In particular, there's usually only a subset of labelled nodes, so training needs to only compute candidate embeddings for those nodes to compare against the ground-truth target array. We currently handle this in full batch methods by having each layer know whether it is the `final_layer` in the model, and do `tf.gather` using a tensor of indices of relevant nodes. This means that the individual full-batch layers have to: - always accept the tensor of indices, even if they won't use it (and, potentially, even if they are being used as part of a model that doesn't ever use it) - do the appropriate `tf.gather` invocation to select the relevant nodes This patch adjusts this approach to make the output filtering a model level concern: layers always compute all the information, and models that need to can do a `tf.gather` call (via the new `layer.misc.GatherIndices`) to select out the relevant output elements. This has a few benefits: - it seems conceptually more "correct" to me, because it's the training/using of a overall model that needs to filter out elements, _not_ the operation of an individual layer - it reduces the amount of code required noticeably, and will reduce if further when we remove the left-over-`final_layer` detection (that is designed to help us migrate). - it makes #1201 easier (in #1205): graph classification doesn't have any output indices to feed into the graph convolution layers, and so this patch makes `GraphConvolution` closer to the behaviour required for that - it may also ease implementing models that incorporate information from multiple convolution layers, because it's easy to make sure all the convolution layers compute embeddings for all nodes See: #1201
huonw
changed the base branch from
feature/model-level-output-indices
to
develop
huonw
force-pushed
the
feature/1201-deduplicate-gcn
branch
from
dd91833 to
62d18e2
Compare
|
Code Climate has analyzed commit bf54b3a and detected 0 issues on this pull request. View more on Code Climate. |
kjun9
reviewed
Apr 8, 2020
huonw
commented
Apr 8, 2020
PantelisElinas
approved these changes
Apr 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ClusterGraphConvolutionandGraphConvolutionwere practically identical, with just some minor differences in the details of the code, andGraphClassificationConvolutiononly differed by:GraphConvolutionin Gather output nodes in full batch models, not the layers #1204GraphConvolutionto handle a batch size > 1 for dense matrices, not sparse ones (yet, since it's harder: Keras's.batch_dotdoesn't seem to support sparse matrices).That is, by building on #1204, this patch can merge all three of our implementations of the GCN layer.
This deprecates
ClusterGraphConvolutionbecause it has been released for a while, but removesGraphClassificationConvolutionentirely, because it has only been released@experimental-y.See: #1201