Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
2fe0a5c
work on optimizing connectivity construction, cleanup Grid
philipc2 Apr 3, 2025
418adbc
update docstrings
philipc2 Apr 4, 2025
9b0e284
update face_face_connectivity
philipc2 Apr 7, 2025
8a31a4d
update face_face_connectivity
philipc2 Apr 7, 2025
39326ba
add derived geometries
philipc2 Apr 7, 2025
ffdd4bb
add geometry module
philipc2 Apr 8, 2025
55b3601
Merge branch 'main' into optimize-face-edges
philipc2 Apr 8, 2025
2a2eee9
Refresh optimize-face-edges branch
cmdupuis3 Jul 10, 2026
19bed33
Rework centroid triangle test
cmdupuis3 Jul 10, 2026
1f8d735
Merge branch 'main' into cmd/merge-OFE
erogluorhan Jul 16, 2026
b69b47f
Add n_nodes_per_face benchmark
cmdupuis3 Jul 17, 2026
b00a124
lazy n_nodes_per_face
cmdupuis3 Jul 17, 2026
e8965ae
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Jul 23, 2026
b9fe4bb
Merge branch 'main' into cmd/merge-OFE
erogluorhan Jul 27, 2026
8b37dc7
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Jul 28, 2026
bd14985
Restore canonical edge ordering in _build_edge_node_connectivity
cmdupuis3 Jul 28, 2026
e7910dc
Sorting algo cleanup
cmdupuis3 Jul 29, 2026
df36057
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Jul 29, 2026
5f0e401
OFE: better parallelism
cmdupuis3 Jul 29, 2026
e5d8f93
Merge branch 'main' into cmd/merge-OFE
erogluorhan Jul 30, 2026
0a623d7
OFE: refactor connectivity and sorting for readability
cmdupuis3 Aug 5, 2026
2334628
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Aug 5, 2026
39dfc2d
OFE: edge corruption warning
cmdupuis3 Aug 5, 2026
51acffb
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 5, 2026
1c65cc9
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Aug 5, 2026
ddfac01
OFE: minor cleanup
cmdupuis3 Aug 6, 2026
ae04596
OFE: typo
cmdupuis3 Aug 6, 2026
4e8c2d8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 6, 2026
5b77ae6
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Aug 6, 2026
523dc6a
Docstring for edge_node and face_edge combined behavior
cmdupuis3 Aug 7, 2026
a5557cd
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Aug 10, 2026
a531a33
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Aug 14, 2026
c87a269
Chunked parallel version of n_nodes_per_face
cmdupuis3 Aug 14, 2026
09fd277
Merge branch 'main' into cmd/merge-OFE
cmdupuis3 Aug 14, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions benchmarks/mpas_ocean.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ def time_to_geodataframe(self, resolution, exclude_antimeridian):


class ConnectivityConstruction(DatasetBenchmark):
number = 1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this doing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to be here for ASV to only run this once. In ASV's default mode, it will run tests multiple times and average them, and if we're worried about caching, we can't have that. So, ASV sees the magical number variable and interprets that as the number of times to run a test.

Smells like Fortran to me, so I get why it raises red flags.


def time_n_nodes_per_face(self, resolution):
self.uxds.uxgrid.n_nodes_per_face

Expand Down
15 changes: 9 additions & 6 deletions test/grid/geometry/test_centroids.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,16 @@ def test_edge_centroids_from_triangle():
grid = ux.open_grid(test_triangle, latlon=False)
_populate_edge_centroids(grid)

centroid_x = np.mean(grid.node_x[grid.edge_node_connectivity[0][0:]])
centroid_y = np.mean(grid.node_y[grid.edge_node_connectivity[0][0:]])
centroid_z = np.mean(grid.node_z[grid.edge_node_connectivity[0][0:]])
edge_nodes = grid.edge_node_connectivity.values

assert centroid_x == grid.edge_x[0]
assert centroid_y == grid.edge_y[0]
assert centroid_z == grid.edge_z[0]
centroid_x = grid.node_x.values[edge_nodes].mean(axis=1)
centroid_y = grid.node_y.values[edge_nodes].mean(axis=1)
centroid_z = grid.node_z.values[edge_nodes].mean(axis=1)
centroid_x, centroid_y, centroid_z = _normalize_xyz(centroid_x, centroid_y, centroid_z)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a substantive change to the test. Can you clarify why this change was needed? Reply here is fine, not trying to say it is wrong, just not understanding yet why it was changed here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so this test was basically passing by coincidence, and whether it passes depends on which edge is at index 0. It also skips normalization. I only found this because prior to implementing sorting, this test was failing, which flagged it as something for Claude to look at, and it just happened to be wrong for another reason. With the new sorting algorithm, the original test would be passing though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand, why does the test need to be changed? It sounds like the original test passes on main and also passes after your changes. Why is it "wrong"?

You mentioned it depends on which edge is at index 0, but that should be known exactly, right? This test is always just acting on this triangle:

    test_triangle = np.array([(0, 0, 0), (-1, 1, 0), (-1, -1, 0)])
    grid = ux.open_grid(test_triangle, latlon=False)

It seems fine to me if part of the test relies on coincidence with that triangle in particular (though it could be nice to have a clarifying comment in that case).

My preference would be to keep the original test unless I can understand why it is wrong. Any of these could work as a way forward: (1) clarify why the original test is wrong, (2) restore the original test but also keep your additions here, so that the test tests more things, or (3) restore the original test and remove your additions here.


nt.assert_array_almost_equal(grid.edge_x.values, centroid_x)
nt.assert_array_almost_equal(grid.edge_y.values, centroid_y)
nt.assert_array_almost_equal(grid.edge_z.values, centroid_z)

def test_edge_centroids_from_mpas(gridpath):
"""Test computed centroid values compared to values from a MPAS dataset."""
Expand Down
114 changes: 113 additions & 1 deletion test/grid/grid/test_connectivity.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
import pytest

import uxarray as ux
from uxarray.constants import INT_FILL_VALUE, ERROR_TOLERANCE
from uxarray.constants import INT_DTYPE, INT_FILL_VALUE, ERROR_TOLERANCE
from uxarray.grid.connectivity import (_populate_face_edge_connectivity, _build_edge_face_connectivity,
_build_edge_node_connectivity, _build_face_face_connectivity,
_populate_face_face_connectivity)
from uxarray.grid.utils import (_adaptive_sort_bucket, _insertion_sort_bucket,
MIN_ADAPTIVE_SORT_SIZE)


def test_connectivity_build_n_nodes_per_face(gridpath):
Expand All @@ -20,6 +22,39 @@ def test_connectivity_build_n_nodes_per_face(gridpath):
# All values should be positive
assert np.all(uxgrid.n_nodes_per_face > 0)

def test_connectivity_n_nodes_per_face_ragged(gridpath):
"""n_nodes_per_face counts non-fill-value nodes on a grid with mixed face sizes."""
uxgrid = ux.open_grid(gridpath("mpas", "QU", "mesh.QU.1920km.151026.nc"))

face_nodes = uxgrid.face_node_connectivity.values
expected = (face_nodes != INT_FILL_VALUE).sum(axis=1).astype(INT_DTYPE)

nt.assert_array_equal(uxgrid.n_nodes_per_face.values, expected)
assert uxgrid.n_nodes_per_face.dtype == INT_DTYPE
# a ragged grid is the point of the test; a uniform one would pass trivially
assert len(np.unique(expected)) > 1

def test_connectivity_n_nodes_per_face_chunked(gridpath):
"""n_nodes_per_face is counted blockwise and stays chunked over ``n_face``."""
path = gridpath("mpas", "QU", "mesh.QU.1920km.151026.nc")
expected = ux.open_grid(path).n_nodes_per_face.values

uxgrid = ux.open_grid(path, chunks={"n_face": 20})
n_nodes_per_face = uxgrid.n_nodes_per_face

# never materialized, and partitioned the same way as its input
assert hasattr(n_nodes_per_face.data, "dask")
assert n_nodes_per_face.chunks == uxgrid.face_node_connectivity.chunks[:1]
nt.assert_array_equal(n_nodes_per_face.values, expected)

def test_connectivity_n_nodes_per_face_chunked_core_dim(gridpath):
"""Chunking ``n_max_face_nodes`` is refused rather than silently rechunked."""
uxgrid = ux.open_grid(gridpath("mpas", "QU", "mesh.QU.1920km.151026.nc"),
chunks={"n_face": 20, "n_max_face_nodes": 2})

with pytest.raises(ValueError, match="n_max_face_nodes"):
uxgrid.n_nodes_per_face.compute()

def test_connectivity_edge_nodes_euler(gridpath):
"""Test edge-node connectivity using Euler's formula."""
uxgrid = ux.open_grid(gridpath("ugrid", "outCSne30", "outCSne30.ug"))
Expand Down Expand Up @@ -63,6 +98,83 @@ def test_connectivity_build_face_edges_connectivity(gridpath):
assert np.all(valid_edges >= 0)
assert np.all(valid_edges < uxgrid.n_edge)

@pytest.mark.parametrize("grid_parts", [("ugrid", "outCSne30", "outCSne30.ug"),
("ugrid", "quad-hexagon", "grid.nc"),
("ugrid", "geoflow-small", "grid.nc")])
def test_connectivity_edge_node_canonical_order(gridpath, grid_parts):
"""Test that constructed edges are numbered in lexicographic node order."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize lexicographic order was supposed to be guaranteed for constructed edges. I see now that the corresponding docstring for _build_edge_node_connectivity has been updated accordingly. Is that change being introduced intentionally by this PR?

If yes, replying with yes here is sufficient and the change looks good to me. In the future if you can mention changes like this too somewhere in the PR overview or as comments in thread, that would have helped with reducing time it takes to review!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sorting was implemented to support compatibility with legacy behavior. If we decide to deprecate that behavior we could throw out all the sorting and revert this a little.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify a bit further? If you can provide at least one example of a legacy behavior this change was implemented to support, that should make it much easier for me to understand.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm talking about is, if you check the test runs on, say, commit 8b37dc7, you will see that five tests consistently fail. These five tests are all failing because the algorithm as it was when I picked up this PR were sorted differently than the legacy tests expect. If we don't care about keeping that legacy behavior, we can remove the sorting algorithm, but I think at this point it's better to keep it and have the sorting because it will be easier to reason about if, in the future, another change to this algorithm changes the sorting order again.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. The commit you linked is in this PR's branch, not a commit on main. To clarify, my question here isn't intended to be "what is the reason for tests about sorting?", I am trying to ask "what is the reason for adding sorting at all?"

Are either of these the situation here?:

  • (A) "Sorting already exists on main, main already produces sorted results, but the optimized algorithms here didn't produce sorted results by default, so we need to add sorting if we want to match the behavior of main; these tests were added to ensure the results after this PR match the results currently on main."
  • (B) "Sorting does not exist on main; this PR introduces sorting."

uxgrid = ux.open_grid(gridpath(*grid_parts))
edge_nodes = uxgrid.edge_node_connectivity.values

# Each edge is stored as an ascending node pair
assert np.all(edge_nodes[:, 0] < edge_nodes[:, 1])

# Edges are numbered lexicographically by that pair, with no duplicates
lexicographic_order = np.lexsort((edge_nodes[:, 1], edge_nodes[:, 0]))
nt.assert_array_equal(lexicographic_order, np.arange(uxgrid.n_edge))
assert len(np.unique(edge_nodes, axis=0)) == uxgrid.n_edge

@pytest.mark.parametrize("sort", [_insertion_sort_bucket, _adaptive_sort_bucket],
ids=["insertion", "adaptive"])
def test_connectivity_bucket_sort(sort):
"""Test that each bucket sort orders its own slice and nothing else.

The bucket sizes straddle ``MIN_ADAPTIVE_SORT_SIZE``: the small ones cannot accumulate
enough shifts to exhaust the budget, so the metered sort stays on its insertion path,
while the 500 element bucket is shuffled far past the budget and falls back to the heap
sort. Keys repeat, since an interior edge reaches its bucket once per adjacent face.
"""
rng = np.random.default_rng(0)

sizes = [5, MIN_ADAPTIVE_SORT_SIZE, MIN_ADAPTIVE_SORT_SIZE + 1, 500]
bounds = np.cumsum([0] + sizes)
n_half_edge = int(bounds[-1])
buckets = list(zip(bounds[:-1], bounds[1:]))

keys = rng.integers(0, 40, n_half_edge).astype(INT_DTYPE)
order = rng.permutation(n_half_edge).astype(INT_DTYPE)

# the key each half edge must still be paired with once the permutation has moved it
key_for = np.empty(n_half_edge, dtype=INT_DTYPE)
key_for[order] = keys

expected_keys = np.concatenate([np.sort(keys[start:end]) for start, end in buckets])

got_keys, got_order = keys.copy(), order.copy()
for start, end in buckets:
shuffle = rng.permutation(end - start)
got_keys[start:end] = got_keys[start:end][shuffle]
got_order[start:end] = got_order[start:end][shuffle]

sort(got_keys, got_order, start, end - start)

nt.assert_array_equal(got_keys, expected_keys)

# sorted keys alone would pass even if the permutation had been scrambled independently
nt.assert_array_equal(key_for[got_order], got_keys)
nt.assert_array_equal(np.sort(got_order), np.arange(n_half_edge))


def test_connectivity_face_edge_positional_alignment(gridpath):
"""Test that face_edge_connectivity[i, j] is the edge between face nodes j and j+1."""
uxgrid = ux.open_grid(gridpath("ugrid", "outCSne30", "outCSne30.ug"))

face_nodes = uxgrid.face_node_connectivity.values
face_edges = uxgrid.face_edge_connectivity.values
edge_nodes = uxgrid.edge_node_connectivity.values

for face_idx, n_edges in enumerate(uxgrid.n_nodes_per_face.values):
for cur in range(n_edges):
start_node = face_nodes[face_idx, cur]
end_node = face_nodes[face_idx, (cur + 1) % n_edges]

expected = sorted((start_node, end_node))
actual = sorted(edge_nodes[face_edges[face_idx, cur]])
assert actual == expected

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

def test_connectivity_build_face_edges_connectivity_fillvalues():
"""Test face-edge connectivity with fill values."""
# Create a simple grid with mixed face types
Expand Down
Loading