Assigning connectivities based on UGRID conventions - #1736
Open
dylannelson wants to merge 3 commits into
Open
Conversation
contains test and explanation for how edge_node_connectivity and edge_face_connectivity interact with the trailing dimension "two"
dylannelson
added
run-benchmark
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have stayed the same:
Benchmarks that have got worse:
|
Member
Author
This branch
Older Versions:
|
dylannelson
changed the title
n2 to two during open_grid
dylannelson
marked this pull request as ready for review
September 11, 2026 16:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1717
Closes #1725
Why two in one? The reason for closing both in one PR is due to them being caused by the same problem. Having 3 dimensions in pre-existing connectivities results in
swap_dimsquietly breaking, causing both the issues seen above. By applying some cleaning both before and afterswap_dimsis called, both issues are solved together. As far as I can see, there is no evidence we would see one of the above errors without the other, and since the solution is in the same location, we may as well solve it once and here. Originally I solved the first one alone, but solving the 2nd would basically require deleting the whole original solution, and replacing it with a more general one. So rather than pushing 1 solution to main, deleting it, and pushing another, I say we just push this single refined solution (assuming it works as intended)Overview
_read_ugridrenamed the trailing dimension offace_node_connectivityton_max_face_nodesand left every other connectivity's equivalent dimension as whatever the source file called it. Readingfesom.mesh.diag.nc, which names those axesn3andn2:face_node_connectivityn3('n_face', 'n_max_face_nodes')face_edge_connectivityn3('n_face', 'n_max_face_nodes')('n_face', 'n_max_face_edges')face_face_connectivityn3('n_face', 'n_max_face_nodes')('n_face', 'n_max_face_faces')edge_node_connectivityn2('n_edge', 'n2')('n_edge', 'two')edge_face_connectivityn2('n_edge', 'n2')('n_edge', 'two')Each connectivity's trailing dimension is now mapped to the name
ugrid.CONNECTIVITYgives it, rather than one being hardcoded and the rest left alone. In this FESOM file (which is read in through the UGRID path) it uses one size-3n3for all three face connectivities.swap_dimscall cannot express all 3, so the first connectivity renames it for the whole dataset and the remainder are renamed per variable.face_node_connectivityis first, so variables that are not connectivity all getn_max_face_nodesExpected Usage
PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: Claude