-
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
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 |
|---|---|---|
|
|
@@ -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): | ||
|
|
@@ -63,6 +65,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. |
||
| 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.
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.
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.
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.