-
Notifications
You must be signed in to change notification settings - Fork 45
Add mesh_layout argument with rectilinear two-step coordinate/connectivity architecture #81
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
6f82d43
63af996
f417962
30ddb19
802b66a
21d33e8
8d18530
4f5a8de
e7e3c03
ef478a2
305e66a
b38fd64
4372a3f
4f48ed3
0b6ff4a
c7bdeac
51ea189
220fa44
edbce82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,9 @@ def create_keisler_graph( | |
| return create_all_graph_components( | ||
| coords=coords, | ||
| m2m_connectivity="flat", | ||
| m2m_connectivity_kwargs=dict(mesh_node_distance=mesh_node_distance), | ||
| mesh_layout="rectilinear", | ||
| mesh_layout_kwargs=dict(grid_spacing=mesh_node_distance), | ||
| m2m_connectivity_kwargs=dict(pattern="8-star"), | ||
|
||
| g2m_connectivity="within_radius", | ||
| m2g_connectivity="nearest_neighbours", | ||
| g2m_connectivity_kwargs=dict( | ||
|
|
@@ -133,10 +135,14 @@ def create_graphcast_graph( | |
| return create_all_graph_components( | ||
| coords=coords, | ||
| m2m_connectivity="flat_multiscale", | ||
| mesh_layout="rectilinear", | ||
| mesh_layout_kwargs=dict( | ||
| grid_spacing=mesh_node_distance, | ||
| refinement_factor=level_refinement_factor, | ||
| max_num_refinement_levels=max_num_levels, | ||
| ), | ||
| m2m_connectivity_kwargs=dict( | ||
| mesh_node_distance=mesh_node_distance, | ||
| level_refinement_factor=level_refinement_factor, | ||
| max_num_levels=max_num_levels, | ||
| pattern="8-star", | ||
| ), | ||
| g2m_connectivity="within_radius", | ||
| m2g_connectivity="nearest_neighbours", | ||
|
|
@@ -217,10 +223,15 @@ def create_oskarsson_hierarchical_graph( | |
| return create_all_graph_components( | ||
| coords=coords, | ||
| m2m_connectivity="hierarchical", | ||
| mesh_layout="rectilinear", | ||
| mesh_layout_kwargs=dict( | ||
| grid_spacing=mesh_node_distance, | ||
| refinement_factor=level_refinement_factor, | ||
| max_num_refinement_levels=max_num_levels, | ||
| ), | ||
| m2m_connectivity_kwargs=dict( | ||
| mesh_node_distance=mesh_node_distance, | ||
| level_refinement_factor=level_refinement_factor, | ||
| max_num_levels=max_num_levels, | ||
| intra_level=dict(pattern="8-star"), | ||
|
||
| inter_level=dict(pattern="nearest", k=1), | ||
| ), | ||
| g2m_connectivity="within_radius", | ||
| m2g_connectivity="nearest_neighbours", | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |||||||||||||||||||||||
| function uses `connect_nodes_across_graphs` to connect nodes across the component graphs. | ||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import warnings | ||||||||||||||||||||||||
| from typing import Iterable | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import networkx | ||||||||||||||||||||||||
|
|
@@ -25,18 +25,26 @@ | |||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| from .grid import create_grid_graph_nodes | ||||||||||||||||||||||||
| from .mesh.kinds.flat import ( | ||||||||||||||||||||||||
| create_flat_multiscale_mesh_graph, | ||||||||||||||||||||||||
| create_flat_singlescale_mesh_graph, | ||||||||||||||||||||||||
| create_flat_multiscale_from_coordinates, | ||||||||||||||||||||||||
| create_flat_singlescale_from_coordinates, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| from .mesh.kinds.hierarchical import ( | ||||||||||||||||||||||||
| create_hierarchical_from_coordinates, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| from .mesh.mesh import ( | ||||||||||||||||||||||||
| create_multirange_2d_mesh_coordinates, | ||||||||||||||||||||||||
| create_single_level_2d_mesh_coordinates, | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| from .mesh.kinds.hierarchical import create_hierarchical_multiscale_mesh_graph | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def create_all_graph_components( | ||||||||||||||||||||||||
| coords: np.ndarray, | ||||||||||||||||||||||||
| m2m_connectivity: str, | ||||||||||||||||||||||||
| m2g_connectivity: str, | ||||||||||||||||||||||||
| g2m_connectivity: str, | ||||||||||||||||||||||||
| m2m_connectivity_kwargs={}, | ||||||||||||||||||||||||
| mesh_layout: str = "rectilinear", | ||||||||||||||||||||||||
| mesh_layout_kwargs: dict = None, | ||||||||||||||||||||||||
| m2m_connectivity_kwargs: dict = None, | ||||||||||||||||||||||||
| m2g_connectivity_kwargs={}, | ||||||||||||||||||||||||
| g2m_connectivity_kwargs={}, | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| m2g_connectivity_kwargs={}, | |
| g2m_connectivity_kwargs={}, | |
| m2g_connectivity_kwargs=None, | |
| g2m_connectivity_kwargs=None, |
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.
I agree with this, also I don't think we should default to rectilinear for the mesh_layout argument. We want people to be explicit about the method they want to use
leifdenby marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
leifdenby marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
leifdenby marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
leifdenby marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
because all m2m_connectivity all now follow the same pattern I think we can simplify the code here. I.e. 1) create the coordinates based on the mesh_layout selected and then 2) create the connectivity between the mesh nodes calling the appropriate function between create_hierarchical_from_coordinates(), create_flat_multiscale_from_coordinates() and create_flat_singlescale_from_coordinates(). What do you think?
leifdenby marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
Copilot
AI
Mar 2, 2026
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.
The rectilinear mesh_layout handling is duplicated across the flat, hierarchical, and flat_multiscale branches (grid_spacing extraction/validation + coordinate graph construction). This repetition makes it easy for defaults/validation to diverge between modes (e.g. the multi-level branches currently differ from the single-level branch). Consider factoring the layout dispatch into a small helper that returns either a single coordinate graph or a list of coordinate graphs, so behavior stays consistent as new layouts are added.
Outdated
Copilot
AI
Mar 2, 2026
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.
In the hierarchical path, refinement_factor (and max_num_refinement_levels) are pulled from mesh_layout_kwargs without defaults/validation, but are then passed directly to create_multirange_2d_mesh_coordinates. If the caller omits refinement_factor, this will pass None as interlevel_refinement_factor and fail inside np.log(...). Either set explicit defaults here (e.g. refinement_factor=3, max_num_refinement_levels=None per the API spec) or raise a clear ValueError when they’re missing/invalid.
| grid_spacing = mesh_layout_kwargs.get("grid_spacing") | |
| refinement_factor = mesh_layout_kwargs.get("refinement_factor") | |
| max_num_refinement_levels = mesh_layout_kwargs.get( | |
| "max_num_refinement_levels" | |
| # Ensure we have a dict to read from, even if the caller passed None. | |
| mesh_layout_kwargs = mesh_layout_kwargs or {} | |
| grid_spacing = mesh_layout_kwargs.get("grid_spacing") | |
| # Default refinement_factor and max_num_refinement_levels per API spec. | |
| refinement_factor = mesh_layout_kwargs.get("refinement_factor", 3) | |
| max_num_refinement_levels = mesh_layout_kwargs.get( | |
| "max_num_refinement_levels", None |
leifdenby marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
Copilot
AI
Mar 2, 2026
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.
In the flat_multiscale path, refinement_factor = mesh_layout_kwargs.get("refinement_factor") can be None, but it’s passed as interlevel_refinement_factor into create_multirange_2d_mesh_coordinates, which will error. Add defaults (and/or validation) for refinement_factor and max_num_refinement_levels before calling the coordinate creation step.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,6 @@ | ||
| from .mesh import create_single_level_2d_mesh_graph | ||
| from .mesh import ( | ||
| create_directed_mesh_graph, | ||
| create_multirange_2d_mesh_coordinates, | ||
| create_single_level_2d_mesh_coordinates, | ||
| create_single_level_2d_mesh_graph, | ||
| ) |
Uh oh!
There was an error while loading. Please reload this page.