Add mesh_layout argument with rectilinear two-step coordinate/connectivity architecture#81
Add mesh_layout argument with rectilinear two-step coordinate/connectivity architecture#81prajwal-tech07 wants to merge 19 commits intomllam:mainfrom
Conversation
…ivity architecture Implement the mesh_layout parameter and refactor mesh graph creation into a two-step process as discussed in mllam#78: 1. Coordinate creation (mesh_layout + mesh_layout_kwargs): - create_single_level_2d_mesh_coordinates() returns nx.Graph with spatial adjacency edges annotated as 'cardinal' or 'diagonal' - create_multirange_2d_mesh_coordinates() returns list of nx.Graph for multi-level meshes with interlevel_refinement_factor stored as graph attr 2. Connectivity creation (m2m_connectivity + m2m_connectivity_kwargs): - create_directed_mesh_graph() converts nx.Graph to nx.DiGraph based on pattern ('4-star' or '8-star') - create_flat_singlescale_from_coordinates() for flat single-scale - create_flat_multiscale_from_coordinates() with intra_level/inter_level sub-dicts for explicit connectivity control - create_hierarchical_from_coordinates() with intra_level/inter_level sub-dicts supporting 'nearest' pattern with k parameter Parameter restructuring: - grid_spacing replaces mesh_node_distance in mesh_layout_kwargs - interlevel_refinement_factor replaces level_refinement_factor - max_num_levels moves to mesh_layout_kwargs - m2m_connectivity_kwargs restructured with intra_level/inter_level sub-dicts Backward compatibility: - Old-style kwargs (mesh_node_distance, level_refinement_factor, max_num_levels in m2m_connectivity_kwargs) are auto-migrated with deprecation warnings - All existing wrapper functions preserved - All existing tests pass unchanged Archetype functions updated to use new parameter scheme: - create_keisler_graph: pattern='8-star' - create_graphcast_graph: intra_level=8-star, inter_level=coincident - create_oskarsson_hierarchical_graph: intra_level=8-star, inter_level=nearest(k=1) Closes mllam#78
Add 46 new tests in test_mesh_layout.py covering: - Coordinate creation (nx.Graph with adjacency_type annotations) - Connectivity creation (4-star vs 8-star pattern filtering) - New API via create_all_graph_components (flat, flat_multiscale, hierarchical) - Backward compatibility with deprecation warnings - Caller dict non-mutation safety - Error handling (unsupported layouts, missing grid_spacing, etc.) - Equivalence between archetype functions and new API Also remove unused imports from base.py (old wrapper functions no longer called directly from the dispatch logic).
… flat_multiscale kwargs - Rename interlevel_refinement_factor → refinement_factor, max_num_levels → max_num_refinement_levels - flat_multiscale uses simple pattern='8-star' (not intra_level/inter_level sub-dicts) - Only hierarchical uses intra_level/inter_level sub-dicts - Update archetypes and backward compat migration messages - Add 36 comprehensive edge case tests (81 total mesh layout tests pass)
There was a problem hiding this comment.
Pull request overview
Introduces the mesh_layout abstraction (starting with rectilinear) to decouple mesh coordinate placement from mesh connectivity construction, updating the mesh creation pipeline to a two-step coordinate/connectivity architecture and adapting archetypes + tests accordingly.
Changes:
- Adds
mesh_layout/mesh_layout_kwargsplumbing to mesh graph creation, including backward-compat kwarg migration with deprecation warnings. - Refactors mesh creation into: coordinate graphs (
nx.Graph) → connectivity graphs (nx.DiGraph) with pattern-based filtering (4-star/8-star). - Adds a comprehensive new test suite validating the new API, behavior, and backward compatibility.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/weather_model_graphs/create/base.py |
Adds mesh_layout dispatch + backward-compat kwarg migration and routes m2m creation through coordinate→connectivity steps. |
src/weather_model_graphs/create/mesh/mesh.py |
Implements coordinate-graph constructors and create_directed_mesh_graph for pattern-based directed connectivity. |
src/weather_model_graphs/create/mesh/kinds/flat.py |
Switches flat single/multiscale connectivity creation to consume coordinate graphs and support pattern. |
src/weather_model_graphs/create/mesh/kinds/hierarchical.py |
Introduces hierarchical connectivity-from-coordinates API with intra/inter-level options and k for nearest inter-level links. |
src/weather_model_graphs/create/mesh/__init__.py |
Exports new mesh coordinate/connectivity helpers. |
src/weather_model_graphs/create/archetype.py |
Updates archetype builders to use the new parameter names and mesh layout plumbing. |
tests/test_mesh_layout.py |
Adds extensive coverage for the two-step architecture, patterns, integration, and backward compatibility. |
CHANGELOG.md |
Documents the new mesh_layout feature under unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m2g_connectivity_kwargs={}, | ||
| g2m_connectivity_kwargs={}, |
There was a problem hiding this comment.
m2g_connectivity_kwargs and g2m_connectivity_kwargs are still using {} as default values. These are mutable defaults and can lead to cross-call state leakage if they are ever mutated inside the function or by callees. Use None defaults and initialize/copy inside the function (as is already done for mesh_layout_kwargs and m2m_connectivity_kwargs).
| m2g_connectivity_kwargs={}, | |
| g2m_connectivity_kwargs={}, | |
| m2g_connectivity_kwargs=None, | |
| g2m_connectivity_kwargs=None, |
There was a problem hiding this comment.
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
| 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" |
There was a problem hiding this comment.
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 |
| 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" | ||
| ) | ||
| if grid_spacing is None: |
There was a problem hiding this comment.
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.
| # --- Step 1: Coordinate creation based on mesh_layout --- | ||
| if mesh_layout == "rectilinear": | ||
| grid_spacing = mesh_layout_kwargs.get("grid_spacing") | ||
| if grid_spacing is None: | ||
| raise ValueError( | ||
| "mesh_layout='rectilinear' requires 'grid_spacing' in " | ||
| "mesh_layout_kwargs (or 'mesh_node_distance' in " | ||
| "m2m_connectivity_kwargs for backward compatibility)." | ||
| ) | ||
| # Compute number of mesh nodes from grid_spacing |
There was a problem hiding this comment.
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.
| if pattern == "4-star": | ||
| # Filter to only cardinal edges (horizontal/vertical) | ||
| edges_to_use = [ | ||
| (u, v) | ||
| for u, v, d in G_undirected.edges(data=True) | ||
| if d.get("adjacency_type") == "cardinal" | ||
| ] | ||
| elif pattern == "8-star": | ||
| # Use all edges (cardinal + diagonal) | ||
| edges_to_use = list(G_undirected.edges()) | ||
| else: | ||
| raise ValueError( | ||
| f"Unknown connectivity pattern: '{pattern}'. " | ||
| "Choose '4-star' or '8-star'." | ||
| ) | ||
|
|
||
| # Create filtered undirected graph with only selected edges | ||
| g_filtered = networkx.Graph() | ||
| g_filtered.add_nodes_from(G_undirected.nodes(data=True)) | ||
| g_filtered.add_edges_from(edges_to_use) | ||
|
|
There was a problem hiding this comment.
create_directed_mesh_graph rebuilds a filtered graph using add_edges_from(edges_to_use) where edges_to_use contains only (u, v) pairs, so edge attributes from the coordinate graph (notably level and adjacency_type) are dropped. This breaks downstream logic that expects level on intra-level edges (e.g. split_graph_by_edge_attribute(..., "level") in create_all_graph_components for hierarchical graphs). Preserve the original edge data when filtering (e.g., build edges_to_use as (u, v, d) triples copied from G_undirected.edges(data=True)), so attributes like level survive into the directed graph.
|
Hi @leifdenby 👋 I've opened this PR implementing the
Copilot left a few review suggestions (mutable defaults, default values for I'm currently also working on #79 (prebuilt mesh support) and #80 (triangular layout), which build on top of this PR. It would be great if you could review this one when you get a chance, so I can keep the follow-up work aligned with your feedback. Thanks! |
leifdenby
left a comment
leifdenby
left a comment
There was a problem hiding this comment.
This is really excellent! Thanks for doing this work. I have added some comments with suggestions for change. There are also some naming refactoring I think we should do (also in the comments). In general I would say this is 95% done though, well done! 🌟
| m2g_connectivity_kwargs={}, | ||
| g2m_connectivity_kwargs={}, |
There was a problem hiding this comment.
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
|
Do you fancy adding a notebook demonstrating the the |
- Rename mesh.py -> coords.py to reflect coordinate/primitive focus - Split mesh creation into two steps: primitive creation (nx.Graph) and directed connectivity creation (nx.DiGraph) - Add create_single_level_2d_mesh_primitive and create_directed_mesh_graph as new public API - Make mesh_layout a required parameter (no default) to force explicit choice - Extract _migrate_deprecated_kwargs() as a separate module-private helper function for planned removal - Use dict-based interface for intra_level/inter_level kwargs in hierarchical mesh creation - Remove redundant default values in base.py, let each mesh kind handle its own defaults - Add _check_required_graph_attributes() validation in flat.py - Group method+kwargs arguments together in archetype functions - Add comprehensive docstrings explaining 4-star/8-star patterns - Preserve backward compatibility with deprecation warnings - Update all tests to pass mesh_layout explicitly - All 81 mesh layout tests passing, 177/179 full suite passing (2 pre-existing Windows PermissionError on temp PNG files)
|
Hi @leifdenby, I've addressed all the review feedback from this PR. Here's a summary of every change: Renaming & API restructuring:
Type annotations & docstrings:
|
leifdenby
left a comment
leifdenby
left a comment
There was a problem hiding this comment.
This is looking really good. In addressing my comments please make a separate commit for each comment you are addressing and paste a link to that commit in your reply. That makes it a lot easier for me to see how you addressed the comment.
In general I don't think we should have this pattern:
def myfunction(arg=None):
if arg is None:
arg = 42
...That hides the fact that the default for arg is actually 42. Instead we should always put the default value in the call signature.
| Updated (mesh_layout_kwargs, m2m_connectivity_kwargs). | ||
| """ | ||
| if "mesh_node_distance" in m2m_connectivity_kwargs and "mesh_node_spacing" not in mesh_layout_kwargs: | ||
| warnings.warn( |
There was a problem hiding this comment.
We use loguru for logging throughout weather-model-graphs please replace this with a loguru logger.warn() call
There was a problem hiding this comment.
Addressed in prajwal-tech07@e7e3c03 — switched _migrate_deprecated_kwargs to use logger.warning() from loguru instead of warnings.warn(). Also updated the backward-compat tests to capture loguru output in prajwal-tech07@305e66a
|
|
||
|
|
||
| def create_multirange_2d_mesh_graphs( | ||
| max_num_levels, xy, mesh_node_distance=3, level_refinement_factor=3 |
There was a problem hiding this comment.
Addressed in prajwal-tech07@21d33e8 — added type annotations to all parameters and also exposed the pattern argument to callers.
| if inter_level is None: | ||
| inter_level = {"pattern": "nearest", "k": 1} | ||
|
|
||
| intra_level_pattern = intra_level.get("pattern", "8-star") |
There was a problem hiding this comment.
I think we should remove these in-line defaults where we map None to something default, as I said in my comment above. Put the default in the actual call signature instead
There was a problem hiding this comment.
Addressed in prajwal-tech07@4f5a8de — removed the None-to-default mapping and put defaults directly in the call signature:
def create_hierarchical_from_coordinates(
G_coords_list: List[networkx.Graph],
intra_level: Dict[str, object] = {"pattern": "8-star"},
inter_level: Dict[str, object] = {"pattern": "nearest", "k": 1},
):|
|
||
| G_all_levels = [] | ||
| for g_coords in G_coords_list: | ||
| g_directed = create_directed_mesh_graph(g_coords, pattern="8-star") |
There was a problem hiding this comment.
should this pattern argument not be exposed to the caller?
There was a problem hiding this comment.
Addressed in the same commit: prajwal-tech07@21d33e8 — create_multirange_2d_mesh_graphs now accepts pattern: str = "8-star" and passes it through to create_directed_mesh_graph.
| ) | ||
|
|
||
| return mesh_graph.create_single_level_2d_mesh_graph(xy=xy, nx=nx, ny=ny) | ||
| return mesh_coords.create_single_level_2d_mesh_graph(xy, nx, ny) |
There was a problem hiding this comment.
keep the call arguments with explicit names
There was a problem hiding this comment.
Addressed in prajwal-tech07@8d18530 — all function calls now use explicit keyword names, e.g. mesh_coords.create_single_level_2d_mesh_graph(xy=xy, nx=nx, ny=ny).
… directed graph)
Add type annotations and expose pattern argument in create_multirange_2d_mesh_graphs.
Each mesh primitive (nx.Graph) encodes spatial adjacency with 'cardinal'/'diagonal'
edge annotations; create_directed_mesh_graph converts to nx.DiGraph with pattern
filtering ('4-star' or '8-star').
Add create_flat_multiscale_from_coordinates and create_flat_singlescale_from_coordinates that take coordinate graphs from the first step. Use explicit kwarg names in all function calls (e.g. mesh_coords.create_single_level_2d_mesh_graph(xy=xy, nx=nx, ny=ny)).
…l_from_coordinates
Instead of using None-to-default mapping pattern, set defaults directly:
intra_level={'pattern': '8-star'}
inter_level={'pattern': 'nearest', 'k': 1}
This makes the actual defaults visible in the function signature.
…ation in base - Use logger.warning() from loguru instead of warnings.warn() in _migrate_deprecated_kwargs - Add mesh_layout + mesh_layout_kwargs parameters for coordinate creation step - Implement two-step process (coordinate creation then connectivity creation) for all three m2m types: flat, hierarchical, flat_multiscale
- All three archetype functions now pass mesh_layout='rectilinear' and mesh_layout_kwargs with mesh_node_spacing - Reorder arguments: coords_crs/graph_crs before connectivity args - Add mesh_layout='rectilinear' to test_create_graph_generic and test_plot
… of DeprecationWarning Since _migrate_deprecated_kwargs now uses loguru logger.warning() instead of warnings.warn(), the tests need to use a loguru StringIO sink to capture and verify the deprecation warning messages.
|
Thank you for the detailed review @leifdenby! I've addressed all your feedback with separate commits as requested. All 177 tests pass. Also, whenever you get a chance, it would be great if you could review PRs #91 and #92 as well. Thanks! |
leifdenby
left a comment
leifdenby
left a comment
There was a problem hiding this comment.
This is really getting there!
One general pattern we should avoid: All default parameter values (pattern, refinement_factor, etc) should not be set inline as follows:
# anti-pattern, we should not do this
def my_function(refinement_factor):
# do something...
refinement_factor = kwargs.get("refinement_factor", 42) # <-- this is bad, hiding default value inline rather than in function signature
my_function(refinement_factor=refinement_factor, ...)Instead we should do this:
def my_function(refinement_factor=42):
# do something
my_function(**kwargs)| mesh_layout_kwargs=dict(mesh_node_spacing=mesh_node_distance), | ||
| m2m_connectivity="flat", | ||
| m2m_connectivity_kwargs=dict(mesh_node_distance=mesh_node_distance), | ||
| m2m_connectivity_kwargs=dict(pattern="8-star"), |
There was a problem hiding this comment.
based on discussions in the last dev meeting there was preference for m2m connectivity not being given explicitly here, but instead we should just let the m2m_connectivity method define its own default (which for flat would be 8-star) and then we don't have to pass in m2m_connectivity_kwargs here
| max_num_refinement_levels=max_num_levels, | ||
| ), | ||
| m2m_connectivity="flat_multiscale", | ||
| m2m_connectivity_kwargs=dict(pattern="8-star"), |
There was a problem hiding this comment.
remove as for flat as commented on https://github.com/mllam/weather-model-graphs/pull/81/changes#r2953045634
| mesh_node_distance=mesh_node_distance, | ||
| level_refinement_factor=level_refinement_factor, | ||
| max_num_levels=max_num_levels, | ||
| intra_level=dict(pattern="8-star"), |
There was a problem hiding this comment.
again suggested that inter-level and intra-level connectivity parameters defined here should be the default assumed for hierarchical similar to what I suggested for flat here: https://github.com/mllam/weather-model-graphs/pull/81/changes#r2953045634
| mesh_node_spacing = mesh_layout_kwargs.get("mesh_node_spacing") | ||
| if mesh_node_spacing is None: | ||
| mesh_node_spacing = mesh_layout_kwargs.get("grid_spacing") | ||
| refinement_factor = mesh_layout_kwargs.get("refinement_factor", 3) |
There was a problem hiding this comment.
these defaults should be in the function definition where they are used rather than inline here.
| hier_kwargs["inter_level"] = inter | ||
| graph_components["m2m"] = create_hierarchical_from_coordinates( | ||
| G_coords_list, | ||
| **hier_kwargs, |
There was a problem hiding this comment.
I think we should just pass in m2m_connectivity_kwargs in here, this constructing a new dict and getting values from it is error prone
… forwarding
- Remove explicit m2m_connectivity_kwargs from all three archetype functions
(keisler, graphcast, oskarsson_hierarchical) — let downstream
from_coordinates functions define their own defaults
- Remove inline pattern = kwargs.get('pattern', '8-star') in base.py flat
and flat_multiscale branches — pass **m2m_connectivity_kwargs directly
- Remove inline refinement_factor default of 3 — conditionally build
primitives_kwargs so function signature defaults are respected
- Simplify hierarchical branch: pass **m2m_connectivity_kwargs directly
instead of constructing a new hier_kwargs dict
Addresses review feedback from leifdenby (Mar 18, 2026)
|
Hi @leifdenby, thanks for the detailed review! I've addressed all your feedback in commit 4f48ed3. Here's a summary of the changes:
All 177 tests pass |
leifdenby
left a comment
leifdenby
left a comment
There was a problem hiding this comment.
Ok, now with this looking so clean I am starting seeing some structure appearing where a bit more refactoring would be nice. In particular, as I wrote in my comment I think we should do away with the "kinds" description for the meshes here in the code to create a clear separation between computing the mesh coordinates and creating the mesh connectivity.
There was a problem hiding this comment.
This is getting there, really starting to look nice. One thing I am wondering is wrt to making it easy to understand how to add apply connectivity approaches (flat, flat_multiscale, hierarchical) when we get new methods for defining the mesh coordinates (via the mesh_layout) argument. In that respect I think we should maybe do the following:
- rename
wmg.create.mesh.kindsaswmg.create.mesh.connectivitysince that makes it clear that the functionality here is about constructing the connectivity after we have the coordinates - remove all explicit mention of
patternin the mesh-connectivity methods in this module. The motivation for this that when we add for exampletriangularmeshes then the pattern to use will change. So insteadcreate_hierarchical_from_coordinates(),create_flat_multiscale_from_coordinates()andcreate_flat_singlescale_from_coordinates()should take a**kwargsdict(which can then containpattern) which will be passed down to the function that actually create the directed edges. What do you think?
| @@ -126,25 +239,123 @@ def create_all_graph_components( | |||
| xy = np.stack(xy_tuple, axis=1) | |||
|
|
|||
| if m2m_connectivity == "flat": | |||
There was a problem hiding this comment.
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?
…e notebook - Rename wmg.create.mesh.kinds -> wmg.create.mesh.connectivity (clearer separation between coordinate creation and connectivity) - Replace explicit 'pattern' param with **kwargs in create_flat_singlescale_from_coordinates and create_flat_multiscale_from_coordinates (future-proofs for triangular and other mesh layouts) - Update imports in base.py and tests/test_mesh_layout.py - Update docs/creating_the_graph.ipynb to use new mesh_layout API - Fix black and isort formatting
|
Hi @leifdenby, thanks for the review! I've addressed all your feedback in commit 0b6ff4a:
Regarding the All 177 tests pass locally |
|
the linting is still failing @prajwal-tech07 :) The easiest way to run is with |
- Fix isort import ordering in base.py - Fix black-jupyter formatting in creating_the_graph.ipynb - Add missing max_num_refinement_levels param to notebook cell - All pre-commit hooks pass: trailing-whitespace, end-of-file-fixer, isort, black, black-jupyter, flake8
|
Thanks @leifdenby! I've pushed the fixes in
Regarding the further refactoring you suggested (separating coordinate creation into a helper in |
|
Hi Leif, I’ve addressed the requested changes and all checks are passing now. Could you please take another look and approve if everything looks good? |
There was a problem hiding this comment.
This is looking good, just that refactor in .create.base remaining now.
To be clear: I am not suggesting that you have to add more helper functions, but you could if you want, e.g. something like create_mesh_coordinates(layout=m2m_layout) and create_mesh_connectivity(connectivity=m2m_connectivity).
What I am suggesting is that you reorder the if-statements. You should only have "Step 1", "Step 2" written once in the final implementation, so that "Step 1" produces a G_mesh_coords object, which can either be single networkx.Graph object or a List[nx.Graph] (add inline type annotation for this) and this object is then in Step 2 passed to the relevant function that applies the selected connectivity operation. That way we have proper separation of concerns so that steps are only written once but the functions used to process each step is dependent on the values of mesh_layout and m2m_connectivity.
Similarly, you should structure creating the kwargs for each so that they are grouped just above where they are used for either coordinate creation or connectivity creation. You could do this inside the two functions if you create them :) Separating the functionality out into two separate functions would reduce the size of create_all_graph_components which could be good.
…ation (Step 2) in base.py Per leifdenby's review: reorder if-statements so that: - Step 1 (coordinate creation) is written once, branching on mesh_layout - Step 2 (connectivity creation) is written once, branching on m2m_connectivity This eliminates ~80 lines of duplicated coordinate-creation code that previously existed inside each of the flat/hierarchical/flat_multiscale branches. The G_mesh_coords variable holds either: - a single nx.Graph (for flat singlescale) - a List[nx.Graph] (for hierarchical and flat_multiscale) Also adds early validation of m2m_connectivity before Step 1 to give a clear NotImplementedError immediately for unsupported values. All 193 tests pass, pre-commit hooks pass.
|
Hi @leifdenby! I've pushed the The
This eliminates ~87 lines of duplicated coordinate-creation code that previously existed inside each of the 3 connectivity branches. The kwargs for each step are now grouped right above where they're used. All 193 tests pass and all pre-commit hooks pass |
Describe your changes
Add
mesh_layoutargument with rectilinear two-step coordinate/connectivity architectureImplements the
mesh_layoutargument for mesh graph creation, starting withrectilinearas the first supported layout. This uses a two-step architecture that separates:nx.Graph) — places mesh nodes on a grid withcardinalanddiagonaledge annotationsnx.DiGraph) — filters edges by pattern ("4-star"or"8-star") and computes edge featuresChanges by file:
base.pymesh_layout(default:"rectilinear") andmesh_layout_kwargsparameters tocreate_single_level_2d_mesh_graphandcreate_multi_level_2d_mesh_graphrefinement_factorandmax_num_refinement_levelsflat_multiscaledispatches with simplepatternkwarg (no sub-dicts)hierarchicaldispatches withintra_level/inter_levelsub-dictsmesh_node_distance,level_refinement_factor,max_num_levels) still work with deprecation warningsgrid_spacingpresence whenmesh_layoutis specifiedflat.pycreate_flat_multiscale_from_coordinates()now accepts a simplepattern="8-star"argument instead of sub-dicts, matching Leif's API specificationarchetype.pycreate_graphcast_graphandcreate_oskarsson_hierarchical_graphto use new parameter names (refinement_factor,max_num_refinement_levels)tests/test_mesh_layout.pyAPI table (per specification in #78):
m2m_connectivitymesh_layoutmesh_layout_kwargsflatrectilinearpattern="8-star"flat_multiscalerectilinearpattern="8-star",refinement_factor=3,max_num_refinement_levels=Nonehierarchicalrectilinearintra_level={"pattern": "8-star"},inter_level={"pattern": "knn", "k": 3},refinement_factor=3,max_num_refinement_levels=NoneMotivation and context:
This lays the groundwork for supporting alternative mesh layouts (e.g., triangular in #80) by cleanly separating coordinate placement from connectivity logic through the
mesh_layoutabstraction.Dependencies:
No new dependencies required. Uses existing
networkx,numpy,scipy.Issue Link
closes #78
Type of change
Checklist before requesting a review
pullwith--rebaseoption 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