-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add containing_rectangle graph connection method for m2g #28
Conversation
Just a thought - what about requiring that the mesh surrounds the grid so that all grid points have 4 decent nns? |
Me and @TomasLandelius discussed this a bit offline and realized the fact that the outermost nodes will always be on the boundary in LAM models, meaning that we don't actually care about the prediction for that node and hence its m2g connectivity pattern does not matter. This means that the long red edge in my first figure above is not really a problem. The "problem" with the second red edge remains. This relates to the question of whether we should place the outermost mesh nodes to surround the grid or (as we do now) inside the bounding box of the grid. This basically boils down to a tradeoff of where we want the irregular connectivity pattern:
Since we realize that we do not care about predicting for the outermost grid nodes (we don't even need to connect to these in m2g really), the first option is likely to prefer. The mesh nodes (also those over the boundary) will impact the prediction also for nodes within the state region. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this extension. The code is in good shape. I have some conceptual questions and one technical comment (mostly for my education I guess).
-
Once we start connecting realistic boundaries the state: is your statement below referring to the nodes at the border of the state or at the border of the boundary(+state)? I don't think we would want irregularities neither in grid nor mesh nodes at the border of the state, right?
The outermost grid nodes having a somewhat strange and irregular connectivity pattern (current situation), or The outermost mesh nodes having a somewhat strange and irregular connectivity pattern
-
Could
containing_rectangle
be used for many shapes in the future (e.g. hex-grid). Will we add seperate functions for each type of grid or call this onecontaining_shape
? -
Looking at the roadmap I saw that graphtools is planned as an additional backend for v0.3.0. The motivation for the two-step approach (within-radius -> containing-rectangle) seems to be performance. I was wondering whether you would still do this in a highly optimized library like graphtools?
I am not complete sure from the roadmap whether this feature was planned for 0.1.1 or 0.2.0? From my side we can merge now and make sure that we add it to the roadmap accordingly.
# Connect to all nodes that could potentially be close enough, | ||
# which is at a relative distance of 1 | ||
rad_graph = connect_nodes_across_graphs( | ||
G_source, G_target, method="within_radius", rel_max_dist=1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this 1.0 and not srt(dx^2 + dy^2)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. This is relative distance, which is relative to the longest edge in (the bottom level of) the source and target graphs. These longest edges will be the diagonal edges in each rectangle in the mesh graph, which have the length sqrt(dx^2 + dy^2)
. So that is exactly the distance of rel_max_dist=1.0.
I added a bit of a clarification about this, and also realized that there was nothing describing the rel_max_dist
argument in the docstring, so fixed that as well.
This was referring to the nodes at the border of the boundary(+state)? I was a bit unspecific (I sometimes think we should just define a full on list of terminology with figures for these things 😄 ). I agree that we would not want any form of irregularities at the border of the state (inner region) itself.
Sadly I think that this is quite specific to these rectangular mesh graphs, so I do not expect that this will trivially work for other mesh node layouts. However, for the hex-grid case (i.e. triangular mesh) I am quite sure that just 3-nn connections will achieve the same thing as this does for rectangular meshes. This just because of the geometry with triangles. The only issue with 3-nn in that case would be at the border of the state+boundary region, but as discussed above that does not really matter as we do not predict there.
I would still do this two-step approach, as this is not needed due to networkx being slow (there is actually not really any networkx functions being used here). This is just inherent to the problem, if you would check each pair of nodes if they should be connected that will always be
Yes, I think this was not on the original road map, but is mentioned in #1 (comment). It should probably go in under some version number though. I think it would be a good idea to create a new release here soon, in particular before #32 is merged in. |
@TomasLandelius did you want to go over this code in more detail or are you happy with the addition? |
@joeloskarsson Sorry for being so slow. I'm happy with this! |
Describe your changes
This PR introduces a new method for connecting a mesh graph to the grid.
Background
In earlier work we have used 4-nearest-neighbors (4nn) to create the m2g connection. This is not terrible, but has its issues (see below), especially for grid cells along the edges (outside the area covered by the mesh graph).
Below is an example of how a 4nn m2g graph can look like, with a few edges highlighted
containing_rectangle
The 4nn method was somewhat inspired by how m2g is created in GraphCast, where each grid node is projected up to the corresponding triangular face containing it in the mesh graph. The
containing_rectangle
method is an attempt at achieving something closer to this for regular quadrilateral mesh graphs.Starting with the end result, m2g edges created using the
containing_rectangle
look likeThe idea of this method is to similarly to GraphCast find the face of the mesh graph containing each grid node and connect the grid nodes with the edges of that face. Since we currently work with a regular layout for the mesh nodes these faces are rectangles, as illustrated below:
Note that this means that grid nodes along the edges will have less than 4 mesh connections.
Definition of edge set
The actual definition of this edge set is as follows: Let$d_x$ be the distance between the columns of mesh nodes in the x-direction and similarly $d_y$ in the y-direction. A mesh nodes with coordinates $(x_m, y_m)$ should be connected to a grid node with coordinates $(x_g, y_g)$ if
To avoid having to check all grid and mesh nodes pairwise for the expression above we in practice start by building a radius graph based on radius$r = \sqrt{d_x^2 + d_y^2}$ . This gives an edge set that is a superset of the
containing_rectangle
edge set. We then filter this edge set using the expression above to get the final set of edges. Trying to build a KD-tree using this distance measure directly seemed a bit over-complicated and this two-step method works fast without any issues.For now this method is only defined for m2g. It is not entirely clear how to do it for g2m, but it should be possible to extend also to those edges.
Issue Link
No relevant issue.
Type of change
Checklist before requesting a review
pull
with--rebase
option if possible).Checklist for reviewers
Each PR comes with its own improvements and flaws. The reviewer should check the following:
Author checklist after completed review
reflecting type of change (add section where missing):
Checklist for assignee