-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Idea for how to define neural-networks operating on graph #35
Comments
So there's a few things that you bring up here, and some are almost orthogonal. W.r.t embedders: I agree that this could be a bit more structured. In some code I've been working with I have a function I struggle a bit to understand the other parts, so would be grateful for some clarifications.
Could you explain how the current implementation looks different from the equations in the paper? I look for example at something like neural-lam/neural_lam/models/base_graph_model.py Lines 147 to 149 in 879cfec
and to me this tells exactly which node sets messages are passed between and what edges to use. Is the idea that you want the logic of the forward pass of the model to be defined in the init function? To me that it makes sense to follow the practice of instantiating the network blocks in init and using them in forward . However, something I think we should change is to break things up into separate nn.Modules (e.g. the encoder, processor and decoder parts), so that the forward pass actually sits in a forward function, rather than something like predict_step .
Have you thought about how this should work with the class hierarchy? I think that is something I fail to see. It makes sense when you write out the Connected to above, my understanding of the
I interpret this as being able to create new GNN layers, is that correct? I think that is very important and something we should think about. If we keep the current function signature from the |
Ok, could you point me to this? Just so we're on the same page: what I thought would be nice to have was a single point where we in a sense register what embedding networks that will be constructed. This could be achieve in different ways, for example
I would prefer option 1 as this would allow us to easily print what embedding networks are initiated, easy to understand how to add more, and would enforce that they work identically. In container type for these graph based models this could be implemented with something like: from torch import nn
class EncodeProcessDecodeGraph:
def __init__(self):
self._embedding_blueprints = {}
self._embedding_networks = {}
def _register_embedder(
self, identifier, n_features_in, n_features_out, kind
):
self._embedding_blueprints[identifier] = dict(
n_features_in=n_features_in,
n_features_out=n_features_out,
kind=kind,
)
def _construct_embedders(self):
for identifier, blueprint in self._embedding_blueprints.items():
n_in = blueprint["n_features_in"]
n_out = blueprint["n_features_out"]
if blueprint["kind"] == "linear_single":
self._embedding_networks[identifier] = nn.Linear(n_in, n_out)
else:
raise ValueError(f"Unknown kind: {blueprint['kind']}")
class KeislerGraph(EncodeProcessDecodeGraph):
def __init__(
self, hidden_dim_size=512, n_grid_features=10, n_edge_features=2
):
super().__init__()
self._register_embedder(
identifier="grid_node",
n_features_in=n_grid_features,
n_features_out=hidden_dim_size,
kind="linear_single",
)
self._register_embedder(
identifier="g2m_and_m2g_edge",
n_features_in=n_edge_features,
n_features_out=hidden_dim_size,
kind="linear_single",
)
self._register_embedder(
identifier="m2m_edge",
n_features_in=n_edge_features,
n_features_out=hidden_dim_size,
kind="linear_single",
)
self._construct_embedders() I will create a separate comment for the message-passing networks |
The Overall I think this looks nice. This could act almost as a wrapper for One thing I am wondering though: Could we not just immediately create each embedder, instead of registering them first and then having a separate call for constructing them? If we have something like |
Here is the neural-lam/neural_lam/models/graph_efm.py Lines 295 to 372 in 89c5ce9
But again, that is a bit orthogonal since it is about how the embedders are applied to grid-input + graph, rather than how they are created. Perhaps of more interest is how the embedders are created in that class: neural-lam/neural_lam/models/graph_efm.py Lines 49 to 138 in 89c5ce9
This is at least collected all in one place, but could be even nicer with something like what's proposed above. |
I've been thinking about whether we could construct the different neural-networks which update the graph. The ideas below are very tentative and I may also have fundamentally misunderstood parts of how this is supposed to work, so all feedback is very welcome.
To my mind there are two types of neural networks we used: 1) MLPs for creating embeddings of node/edge features into a common-sized latent-space across all embeddings and 2) for updating node embeddings using message-passing.
What I would like to achieve is code that:
I think there are basically three steps to this:
Below is my code example that tries to encapsulate all the information I think is needed to later in the code actually instantiate the neural-network models that do the numerical operations.
A few notes on this to explain what is going on:
dict
in effect define filters that select edgesnn.Sequential
but is isn't of course because its not like the output from one step feeds into the next.I hope this isn't total nonsense @joeloskarsson and @sadamov 😆 just trying to get the ball rolling
The text was updated successfully, but these errors were encountered: