-
Notifications
You must be signed in to change notification settings - Fork 55
Optimized connectivity: Simultaneous face_edges and edge_nodes #1560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2fe0a5c
418adbc
9b0e284
8a31a4d
39326ba
ffdd4bb
55b3601
2a2eee9
19bed33
1f8d735
b69b47f
b00a124
e8965ae
b9fe4bb
8b37dc7
bd14985
e7910dc
df36057
5f0e401
e5d8f93
0a623d7
2334628
39dfc2d
51acffb
1c65cc9
ddfac01
ae04596
4e8c2d8
5b77ae6
523dc6a
a5557cd
a531a33
c87a269
09fd277
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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): | ||
|
|
@@ -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")) | ||
|
|
@@ -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.""" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?:
|
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing?
There was a problem hiding this comment.
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
numbervariable 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.