Optimized connectivity: Simultaneous face_edges and edge_nodes by cmdupuis3 · Pull Request #1560 · UXARRAY/uxarray

cmdupuis3 · 2026-07-10T22:39:24Z

Related to #1180

Supercedes #1195

Overview

This set of changes optimizes face_edge, edge_node, and face_face connectivity. face_edge and edge_node connectivity are combined into one routine, while face_face is optimized stand-alone.

PR Checklist

General

An issue is linked created and linked
Add appropriate labels
Filled out Overview and Expected Usage (if applicable) sections

Testing

Adequate tests are created if there is new functionality
Tests cover all possible logical paths in your function
Tests are not too basic (such as simply calling a function and nothing else)

Documentation

Docstrings have been added to all new functions
Docstrings have updated with any function changes
Internal functions have a preceding underscore (_) and have been added to docs/internal_api/index.rst

cmdupuis3 · 2026-07-10T22:50:02Z

I think all the hard parts of the merge are done.

At this point, the only failures seems to be sorting issues.

hongyuchen1030 · 2026-07-11T18:39:11Z

I think all the hard parts of the merge are done.

At this point, the only failures seems to be sorting issues.

@cmdupuis3 Thanks for your work, I will review it as soon as possible.

And do you have any idea why the CIs are all failing?

cmdupuis3 · 2026-07-11T20:28:20Z

The CI is failing because the new algorithm returns the results in a different order.

I was thinking we can add some sorting mechanism, at least for legacy behavior. Phillip was evidently aware of this issue too. It depends on if you need to support that... Personally I'd be okay with just changing it, but I think you would be a better judge of the situation.

cmdupuis3 · 2026-07-17T22:26:12Z

Altering njit and .values here runs the risk of conflicting with #1583. I'm thinking we can scope it so that .values cleanup in connectivity.py is allowed on this PR and everywhere except connectivity.py belongs to #1583.

The optimized edge builder deduped half edges with a numba hash map, which numbered edges in first-encounter order. Edges had previously been numbered lexicographically by their (min_node, max_node) pair, as a side effect of the np.unique(..., axis=0) the hash map replaced. Global edge index is a public identity: it indexes edge_lon/edge_lat, edge centered data variables, and edge_node_distances, so renumbering silently re-pairs user data with different physical edges. It also broke the five TestQuadHexagon connectivity tests, which assert on edge_node, face_edge, node_edge, edge_face and face_face -- all the same renumbering cascading through the derived connectivities. Sort as the dedup mechanism instead of hashing. Node indices are dense integers in [0, n_node), so a counting sort buckets the half edges by their first node without any comparisons, and sorting each bucket by its second node leaves the duplicates adjacent -- the dedup then falls out of the same walk. Buckets hold one entry per edge incident to a node, so on a real mesh they are tiny (node degree, typically under ten) and an insertion sort finishes them. A bucket above MAX_INSERTION_SORT_SIZE is heap sorted so that a degenerate mesh cannot degrade the build quadratically; np.argsort is deliberately not used there, as numba's implementation degrades badly on structured input. Half edges are identified throughout by their flat face_node_connectivity index, which is also the face_edge_connectivity slot they are written back to, so the sort needs a single permutation array and no mapping back. This is faster and leaner than the hash map it replaces. On a synthetic one million face quad mesh, measured by peak RSS rather than tracemalloc, which does not observe numba's typed dict allocations: dict build 397.3 ms 239.5 MB bucket sort 85.1 ms 91.6 MB The five legacy tests now pass unchanged. Adds order invariant coverage for the canonical ordering and the face_edge positional contract, plus high degree nodes either side of the insertion sort threshold. Also casts n_nodes_per_face back to INT_DTYPE, so that the builder is not compiled a second time for int64, and restores a blank line dropped between two top level functions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

for more information, see https://pre-commit.ci

erogluorhan

Please see below a few comments:

Sevans711

All of my prior comments have been resolved and no glaring issues popped out at me from a final read-through, so I'm happy to approve!

rajeeja · 2026-09-02T18:54:49Z

Going through this in pieces, will have a few more comments later today. Good PR overall and happy to be reviewing it.

rajeeja · 2026-09-02T18:55:24Z

+            assert actual == expected
+
+        # Remaining slots stay padded
+        assert np.all(face_edges[face_idx, n_edges:] == INT_FILL_VALUE)


@cmdupuis3 The new ValueError in _populate_edge_node_connectivity can't be reached from any caller, so it's untested and shows as uncovered. This MPAS mesh already loads with edge_node_connectivity in _ds, so it pins the guard without a new fixture.

Suggested change

assert np.all(face_edges[face_idx, n_edges:] == INT_FILL_VALUE)

assert np.all(face_edges[face_idx, n_edges:] == INT_FILL_VALUE)

def test_connectivity_edge_node_refuses_to_overwrite(gridpath):

"""Test that rebuilding edges on a grid that already has them is refused."""

from uxarray.grid.connectivity import _populate_edge_node_connectivity

# This mesh supplies verticesOnEdge, so the grid loads with edges already present

uxgrid = ux.open_grid(gridpath("mpas", "QU", "mesh.QU.1920km.151026.nc"))

assert "edge_node_connectivity" in uxgrid._ds

with pytest.raises(ValueError, match="already has"):

_populate_edge_node_connectivity(uxgrid)

rajeeja · 2026-09-03T17:14:04Z

Take outCSne30.ug. Renumber its face_edge_connectivity edge ids, put on a fresh grid with
no edge_node_connectivity. Legal UGRID.

Touch grid.n_edge. Main: 5400/5400 rows survive. This PR: 5400/5400 overwritten. No error,
no warning.
Cause: the PR builds both connectivities in one pass, so _populate_edge_node_connectivity
now writes two arrays. The guard at connectivity.py:173 still checks one.
Second write at connectivity.py:200 walks past it.
The guard's reasoning is right. Constructed edges are lexicographic and won't match file
order, which is what its own error message says. It just needs to cover both variables the
function writes now.
Main isn't correct here either, it keeps the file's array while the new
edge_node_connectivity is lexicographic, so the two disagree silently.
Untestable today: no mesh under test/meshfiles has face_edge_connectivity without
edge_node_connectivity. Same hole as test_connectivity.py:176, one variable over.

Two smaller things in utils.py:529-536. The comment on MIN_ADAPTIVE_SORT_SIZE = 16 says a
bucket that size can't exceed the shift budget, which holds only because
MAX_SHIFTS_PER_EDGE = 8: max inversions are size(size-1)/2 against a budget of 8*size, so
size <= 17. That coupling isn't written down or asserted anywhere, and dropping MAX to 4
makes the comment false at size 9.

And test_connectivity_bucket_sort says the 500 bucket falls back to the heap sort, which it
does, but the test only checks the final order and insertion sort alone would produce the
same. Raise MAX_SHIFTS_PER_EDGE and the heap path goes uncovered with the test still green.
_heap_sort_bucket isn't imported by any test.

cmdupuis3 · 2026-09-03T17:42:28Z

@rajeeja Wait, so if I understand correctly, this issue is more that the guards at call sites don't check both connectivities, so without having if "edge_node_connectivity" in grid._ds or "face_edge_connectivity" in grid._ds:, we still have the possibility of desynching between these two connectivity variables, right? It would just be a different mode of desynch than I was thinking of.

I'll see about consolidating the two sorting parameters into one and moving it to constants.py.

_populate_edge_node_connectivity writes both edge_node_connectivity and face_edge_connectivity, and numbers both in the constructed (lexicographic) edge order. The guard only covered edge_node_connectivity, so a grid holding a file-order face_edge_connectivity and no edge_node_connectivity passed every caller's check and had the stored variable silently renumbered on the first access to n_edge or edge_node_connectivity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

rajeeja · 2026-09-04T16:19:24Z

Yes, that covers both writes now, so the n_edge touch fails loud.

cmdupuis3 · 2026-09-04T17:15:49Z

@rajeeja As per a convo yesterday, @erogluorhan and I think the edge_node indexing situation is still a bit confusing, so I want to rethink it slightly before we commit to merging.

erogluorhan

Great!

philipc2 and others added 9 commits April 3, 2025 01:22

work on optimizing connectivity construction, cleanup Grid

2fe0a5c

update docstrings

418adbc

update face_face_connectivity

9b0e284

update face_face_connectivity

8a31a4d

add derived geometries

39326ba

add geometry module

ffdd4bb

Merge branch 'main' into optimize-face-edges

55b3601

Refresh optimize-face-edges branch

2a2eee9

Rework centroid triangle test

19bed33

cmdupuis3 self-assigned this Jul 10, 2026

cmdupuis3 added the improvement Improvements on existing features or infrastructure label Jul 10, 2026

cmdupuis3 mentioned this pull request Jul 10, 2026

DRAFT: Optimize Connectivity Construction #1195

Closed

cmdupuis3 requested a review from hongyuchen1030 July 10, 2026 22:41

cmdupuis3 added scalability Related to scalability & performance efforts and removed improvement Improvements on existing features or infrastructure labels Jul 10, 2026

Merge branch 'main' into cmd/merge-OFE

1f8d735

erogluorhan reviewed Jul 16, 2026

View reviewed changes

Comment thread uxarray/grid/connectivity.py Outdated

cmdupuis3 added 2 commits July 17, 2026 15:56

Add n_nodes_per_face benchmark

b69b47f

lazy n_nodes_per_face

b00a124

cmdupuis3 and others added 4 commits July 22, 2026 21:46

Merge branch 'main' into cmd/merge-OFE

e8965ae

Merge branch 'main' into cmd/merge-OFE

b9fe4bb

Merge branch 'main' into cmd/merge-OFE

8b37dc7

Sevans711 mentioned this pull request Jul 29, 2026

clean up github PR form template #1608

Merged

3 tasks

Sorting algo cleanup

e7910dc

cmdupuis3 added the run-benchmark Run ASV benchmark workflow label Jul 29, 2026

[pre-commit.ci] auto fixes from pre-commit.com hooks

5c4d2dc

for more information, see https://pre-commit.ci

cmdupuis3 requested a review from rajeeja August 21, 2026 21:01

Merge branch 'main' into cmd/merge-OFE

764f6ae

erogluorhan self-requested a review August 25, 2026 20:20

erogluorhan reviewed Aug 25, 2026

View reviewed changes

Comment thread uxarray/grid/connectivity.py Outdated

Comment thread uxarray/grid/connectivity.py Outdated

Comment thread uxarray/grid/connectivity.py Outdated

cmdupuis3 and others added 2 commits August 27, 2026 19:27

Merge branch 'main' into cmd/merge-OFE

fb9a9d0

OFE: clarify edge_node sorting gotcha

2d09dde

Sevans711 approved these changes Aug 31, 2026

View reviewed changes

cmdupuis3 and others added 7 commits September 1, 2026 10:51

OFE: change edge_node error logic slightly

6672b69

OFE: edge_node comment sync

ccfbaff

Merge branch 'main' into cmd/merge-OFE

a78c4c3

Merge branch 'main' into cmd/merge-OFE

0edefc3

OFE: edge_node error logic

ad212cc

OFE: remove f

7557794

Merge branch 'main' into cmd/merge-OFE

53447f8

rajeeja reviewed Sep 2, 2026

View reviewed changes

cmdupuis3 force-pushed the cmd/merge-OFE branch from 545000d to c1fae0e Compare September 3, 2026 17:45

cmdupuis3 and others added 2 commits September 3, 2026 14:03

OFE: consolidate sorting parameters

f06e705

Merge branch 'main' into cmd/merge-OFE

cb9ac0f

Merge branch 'main' into cmd/merge-OFE

f609282

cmdupuis3 and others added 2 commits September 4, 2026 17:26

OFE: last bits on edge_node mixed state

5d0bda7

Merge branch 'main' into cmd/merge-OFE

4c6889b

erogluorhan approved these changes Sep 8, 2026

View reviewed changes

erogluorhan merged commit a1eeb0e into UXARRAY:main Sep 8, 2026
16 checks passed

-        assert np.all(face_edges[face_idx, n_edges:] == INT_FILL_VALUE)
+        assert np.all(face_edges[face_idx, n_edges:] == INT_FILL_VALUE)
+def test_connectivity_edge_node_refuses_to_overwrite(gridpath):
+    """Test that rebuilding edges on a grid that already has them is refused."""
+    from uxarray.grid.connectivity import _populate_edge_node_connectivity
+    # This mesh supplies verticesOnEdge, so the grid loads with edges already present
+    uxgrid = ux.open_grid(gridpath("mpas", "QU", "mesh.QU.1920km.151026.nc"))
+    assert "edge_node_connectivity" in uxgrid._ds
+    with pytest.raises(ValueError, match="already has"):
+        _populate_edge_node_connectivity(uxgrid)

Conversation

cmdupuis3 commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

PR Checklist

Uh oh!

cmdupuis3 commented Jul 10, 2026

Uh oh!

hongyuchen1030 commented Jul 11, 2026

Uh oh!

cmdupuis3 commented Jul 11, 2026

Uh oh!

Uh oh!

cmdupuis3 commented Jul 17, 2026

Uh oh!

erogluorhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sevans711 left a comment

Choose a reason for hiding this comment

Uh oh!

rajeeja commented Sep 2, 2026

Uh oh!

rajeeja Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

rajeeja commented Sep 3, 2026

Uh oh!

cmdupuis3 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajeeja commented Sep 4, 2026

Uh oh!

cmdupuis3 commented Sep 4, 2026

Uh oh!

erogluorhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cmdupuis3 commented Jul 10, 2026 •

edited

Loading

cmdupuis3 commented Sep 3, 2026 •

edited

Loading