feat: add option to crop mesh nodes to convex hull of grid coordinates

[AdMub]

Describe your changes

This PR adds a crop_to_convex_hull boolean parameter to create_single_level_2d_mesh_graph and create_multirange_2d_mesh_graphs.

When set to True, the function computes the scipy.spatial.ConvexHull of the provided xy coordinates and uses a scipy.spatial.Delaunay triangulation to efficiently identify and remove any generated mesh nodes that fall outside the spatial boundary of the actual data. This prevents the generation of "dead space" nodes in the corners of bounding boxes when working with irregular or circular spatial datasets.

Testing performed:

The implementation was verified locally using two distinct spatial datasets to ensure the pruning logic correctly identifies the data perimeter:

• Circle Test: Using a circular data cluster, the standard rectangular mesh generated 400 nodes, while the convex hull cropped mesh generated 316 nodes (successfully pruning 84 empty-space nodes).

• Irregular Shape Test: To test robustness against more complex boundaries, an irregular "heart-shaped" dataset was used. The standard mesh generated 625 nodes, which was successfully pruned to 419 nodes.

Visualization

The plot below demonstrates the "Before" (standard bounding box) and "After" (convex hull cropped) results for the irregular test case. Note how the mesh nodes are strictly contained within the data boundary:

Issue Link

Closes #40

Type of change

• 🐛 Bug fix (non-breaking change that fixes an issue)

• ✨ New feature (non-breaking change that adds functionality)

• 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)

• 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

• My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).

• I have performed a self-review of my code

• For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values

• I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code

• I have updated the documentation to cover introduced code changes

• I have added tests that prove my fix is effective or that my feature works

• I have given the PR a name that clearly describes the change, written in imperative form (context).

• I have requested a reviewer and an assignee (assignee is responsible for merging)

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

• the code is readable

• the code is well tested

• the code is documented (including return types and parameters)

• the code is easy to maintain

Author checklist after completed review

• I have added a line to the CHANGELOG describing this change, in a section reflecting type of change (add section where missing)

Checklist for assignee

• PR is up to date with the base branch

• the tests pass

• author has added an entry to the changelog (and designated the change as added, changed or fixed)

[joeloskarsson]

Please add some suitable tests for this that will be run in the CI as well.

[joeloskarsson]

This option needs to be made available for all types of graphs, and when making them using the different archetypes. You can have a look at my prototype implementation on https://github.com/joeloskarsson/weather-model-graphs/tree/mesh_chull_filtering for some inspiration.

[AdMub]

Hi @joeloskarsson, thank you for the review!

Looking at your mesh_chull_filtering prototype branch is incredibly helpful for understanding the architecture. I will expand the crop_to_convex_hull parameter so it correctly cascades down through the various archetype builders.

I will also add comprehensive tests for the CI pipeline to verify that the Delaunay spatial cropping logic is accurately identifying and removing the out-of-bounds nodes.

I'll push the updated commits to this branch shortly!

[leifdenby]

I'm adding this to milestone v0.4.0 since the issue it is resolving (#40) is already on the roadmap for that release. We can always revisit whether to postpone this for a later release

[leifdenby]

can I add you @joeloskarsson as assignee to this PR?

[AdMub]

Hi @joeloskarsson and @leifdenby,

I have pushed the requested updates! The crop_to_convex_hull parameter has now been fully integrated across the graph archetypes, and I've added a comprehensive test suite to verify the Delaunay spatial cropping logic.

@joeloskarsson — I would love to get your eyes on the updated implementation whenever you have a moment. I have re-requested a review.
@leifdenby — Thank you for adding this to the v0.4.0 milestone! Could one of you please approve/trigger the CI workflows so the new tests can run?

[leifdenby]

Looking good! Some comments for suggestions

[leifdenby]

need line in docstring that explains what crop_to_convex_hull does

[leifdenby]

I don't see where you are suggesting changes to the functions that create the archetypes :) So I think this will fail. Also, I think this argument should be related to either g2m m2m or something like that. Maybe it actually belongs with the mesh_layout/mesh_layout_kwargs being introduced in #81?

[AdMub]

Hi @leifdenby, thank you for the approval and the great catches!

You are completely right about the failing test—I updated the underlying mesh builders but missed exposing the parameter in the top-level archetype functions. I've gone ahead and added the missing docstring to create_single_level_2d_mesh_graph locally.

Regarding PR #81: I completely agree! Since crop_to_convex_hull is fundamentally a mesh layout constraint, it makes perfect sense to group it inside mesh_layout_kwargs rather than bloating the top-level archetype signatures with more standalone booleans.

Would you prefer I update this PR to look for crop_to_convex_hull inside mesh_layout_kwargs now, or should we wait for #81 to merge first so I can rebase and integrate it cleanly?

[leifdenby]

Would you prefer I update this PR to look for crop_to_convex_hull inside mesh_layout_kwargs now, or should we wait for #81 to merge first so I can rebase and integrate it cleanly?

Let's get #81 in first so you don't have to do double work :)

[AdMub]

Sounds perfect, Leif! I'll put a pin in this and keep an eye on #81. Once that merges, I'll sync my branch, shift the crop_to_convex_hull logic inside mesh_layout_kwargs, add the missing docstrings, and push the clean version to get the CI green!

[prajwal-tech07]

Hey @AdMub , just a heads-up that PR #81 has now been merged into main!

Since you were waiting on that merge before syncing your branch and pushing the updated version , that's now unblocked! It would be great if you could rebase off the updated main, move crop_to_convex_hull into mesh_layout_kwargs as @leifdenby suggested, add the docstring to create_single_level_2d_mesh_graph, and push the clean version to get CI green.

This PR is on the v0.4.0 milestone and it's a blocker for the release, so getting it moving again would really help. If you're currently busy or unable to continue, no worries at all , just let us know and I can take this forward so we don't hold up the v0.4.0 release. Thanks for all the work so far!

[AdMub]

Hi @prajwal-tech07, thanks for the heads-up! I am on it.
I will rebase against the updated main, resolve the merge conflicts in mesh.py, and shift crop_to_convex_hull into mesh_layout_kwargs as @leifdenby suggested. I'll also make sure the docstrings are updated.
I'll push the clean version shortly so we can get the CI green and unblock the v0.4.0 release! Thanks for keeping an eye on this.

[leifdenby]

@AdMub thanks for working on this! There is just one merge conflict to resolve, do you think you would have time to look at this? Once that is done I will get the tests to run through and give this a review :)

Thanks again!

[AdMub]

Hi @leifdenby, apologies for that! main must have moved forward a bit. I will pull the absolute latest, resolve that last conflict in mesh.py, and push it up right now!

[joeloskarsson]

Happy to review this again when up to date with main. Please re-request a review from me.

[joeloskarsson]

Nice, but should not be merged :)

[prajwal-tech07]

Hi @AdMub! I took a look at why the CI is failing - there are two separate issues:

1. The tests job fails at collection (this is the main blocker). There are some debug/scratch files committed at the repo root that shouldn't be in the PR:

test_issue_44.py - imports weather_model_graphs.create.mesh.kinds.flat, which doesn't exist on this branch → ModuleNotFoundError, so pytest aborts collection and the whole suite fails before any test runs.
test_issue_40.py, visualize_fix_40.py, convex_hull_verification.png - these look like local debugging artifacts too.
Could you remove those four from the repo root? The proper test already lives in tests/test_graph_creation.py, which is the right place.

2. After removing those, the actual feature test still fails. test_convex_hull_cropping errors with:

TypeError: create_keisler_graph() got an unexpected keyword argument mesh_layout_kwargs
It looks like crop_to_convex_hull / mesh_layout_kwargs is added to the lower-level mesh functions but isn't threaded through the archetype API (create_keisler_graph → create_all_graph_components), so the feature can't be reached the way the test calls it. That plumbing needs to be connected.

One bigger-picture thing: the cropping code is added as a new src/.../mesh/mesh.py, but main reorganized mesh creation in #81 - so it'd be worth integrating into the current structure rather than a parallel module. Happy to help if useful!

[leifdenby]

I completely agree with you, Joel, that it would be highly valuable to support multiscale graphs. Shifting that merging logic from index-based to spatial/position-based is definitely doable. However, since it requires altering the core connectivity assumptions of flat_multiscale, I agree with Prajwal that it's best handled as a dedicated follow-up PR to avoid scope creep on this one.

@AdMub might it be useful to add an inline comment for where this change would need to be made down the line to generalise the implementation?

[AdMub]

Hi @leifdenby, that is a great suggestion.
I have just pushed a commit adding a detailed TODO comment right above the ValueErrorin base.py. It points specifically to create_flat_multiscale_from_coordinates and documents the need to refactor the index-based array reshaping (nx, ny, 2) into a position-based spatial merge.

That should leave a very clear breadcrumb for the follow-up PR. Let me know if that looks good to go!

[leifdenby]

Hi @leifdenby, that is a great suggestion. I have just pushed a commit adding a detailed TODO comment right above the ValueErrorin base.py. It points specifically to create_flat_multiscale_from_coordinates and documents the need to refactor the index-based array reshaping (nx, ny, 2) into a position-based spatial merge.

That should leave a very clear breadcrumb for the follow-up PR. Let me know if that looks good to go!

great! Thank you 🌟 Do you fancy typing up an issue too describing the follow-up that should be done after we've merged this PR? It is good you implemented the exception to make it clear that "cropping to convex hull" doesn't work for flat-multiscale graphs.

I think something we're missing here is a jupyter notebook that demonstrates this new functionality. What it makes sense to introduce a subsection to the existing notebooks? Or do you think it would be good with a new separate notebook? I think if we don't add a notebook here this feature might simply go unnoticed which would be a shame.

[leifdenby]

One last suggestion :) Will you also add an entry to the CHANGELOG explaining your contribution? Thanks!

[AdMub]

"Hi @leifdenby, that makes perfect sense! crop_to_grid_nodes_convex_hull is definitely more explicit and leaves no room for ambiguity.
I have done a global find-and-replace to update the argument name across the codebase, tests, and the new notebook. I also synced with main and resolved the merge conflict in the CHANGELOG to make sure the new feature is documented correctly.
Pushed and running CI now. I think this should be the final piece of the puzzle for v0.4.0!

[leifdenby]

Hmm... I think something has gone wrong in the notebook or the implementation (see my comment).

Also, I think we should not expose the whole mesh_layout_kwargs in the archtype functions, let me know what you think :)

[leifdenby]

I don't think we should expose the whole mesh_layout_kwargs here, doing that exposes a lot of complexity the most people will not need and we will have to greatly increase the content of the docstring. Instead if you would like to expose crop_to_grid_nodes_convex_hull through the archetype functions I would expose just that argument and insert it into the mesh_layout_kwargs, just like what we do for mesh_node_spacing

[leifdenby]

the pruned mesh nodes appear identical to the mesh nodes before pruning, did you inadvertently introduce a bug in your implementation here? :)

[AdMub]

Hi @leifdenby, you were 100% right on both counts!
Because I was passing mesh_layout_kwargs as a dictionary from the outside, the internal archetype logic was overwriting it, which completely bypassed the cropping in the notebook plot. Great catch!
I have refactored archetype.py so that crop_to_grid_nodes_convex_hull is exposed explicitly as a boolean on the archetype signatures. The functions now safely pack it into mesh_layout_kwargs internally.
I've also updated the notebook to use the explicit boolean argument, and the pruned mesh visualization is now rendering perfectly. Pushed and running CI!

[leifdenby]

Nearly there!

[leifdenby]

can you revert the change here so that rather than creating a dict for the kwargs and calling it kwargs here we create the dict inline as before?

[leifdenby]

same comment as above, create the dict inline where passing arguments to create_all_graph_components rather than creating the intermediate kwargs variable

[leifdenby]

as above

[leifdenby]

@AdMub would you be able to reply this comment?

[leifdenby]

You've deleted a changelog entry here, and you need to add a changelog entry for your own work too :)

[leifdenby]

I've moved this feature to the next release milestone (v0.5.0) so that we can get v0.4.0 out

[AdMub]

Hi @leifdenby,
Good call on inlining those dictionaries! I have reverted archetype.py so the mesh_layout_kwargs are explicitly constructed inline within the create_all_graph_components calls, just like before.
I also completely reset the CHANGELOG.md to match main and carefully re-inserted my single line to ensure nothing else got deleted this time. All pushed and ready for v0.5.0!