Cleanup TDE winding edits

[jploveless]

Addressing comments left for me in #523. The main changes are:

• Checking the functionality of the standalone enforce_ccw_tde_winding.py, which now writes updated .msh files for any meshes that contain any number of CW-wound elements (but does not create a new file if all elements are already CCW).
• Improved logging in mesh.py

I'm a little unclear about some of the issues flagged:

celeri/scripts/enforce_ccw_tde_winding.py:113 formats mesh['n_tde'], which the script never assigns, so the mixed-winding branch raises KeyError: 'n_tde'. That is the branch a mesh like this one takes (1 triangle downward, 1840 upward), so the script crashes on exactly the kind of input it exists to fix, and writes no output file. Verified by running it on tests/data/mesh/test_western_north_america_cascadia.msh. A uniformly clockwise mesh — a Japan BEL, say — takes the elif branch and works, which is presumably why it went unnoticed.

Maybe there was some cleanup of enforce_ccw_tde_winding.py done, but this statement is inaccurate. I did update the logging of this script, and I changed the end of it so that no updated file is written if there's no reordering of nodes, but it works as I'd expect for both uniformly CW meshes and mixed-winding meshes.

celeri/mesh.py still says "The file's winding is kept as is; report it rather than change it", and its mixed-winding warning still tells the user to "Reorder those triangles in the mesh file".
MeshConfig's docstring and celeri/operators.py repeat the same claim.

I fixed the reported logging in mesh.py, because it makes sense for no report about changing the file to be made when the nodes of the mesh dict are being modified, but I cannot find reference to this outdated warning in MeshConfig or celeri/operators.py. Again, maybe this is something Claude cleaned up in one of the #523 commits?

[brendanjmeade]

@jploveless One test is failing, and here's the Copilot diagnosis:

---

I'll check the test failures for this PR.Let me check for workflow runs and job status:Let me check the workflow runs for this PR:Let me check the workflow runs for this PR:The "Test" workflow (run 36475875030) is failing. Let me get the details of which specific test jobs failed:The failing test is test_mixed_winding_is_reported_and_rewound in tests/test_mesh_geometry.py.

The test failure shows:

FAILED tests/test_mesh_geometry.py::test_mixed_winding_is_reported_and_rewound - assert False
 +  where False = any(<generator object test_mixed_winding_is_reported_and_rewound.<locals>.<genexpr> at 0x...>)

Let me look at this test to understand what it's checking:The test test_mixed_winding_is_reported_and_rewound is failing because the assertion on line 219-222 expects a warning message containing "mixed vertex winding" and the ratio of rewound triangles, but it's not finding it.

Let me look at the mesh.py file to see what warning is actually being logged:Looking at the test failure, I can see the issue. The test test_mixed_winding_is_reported_and_rewound on line 207 is failing with:

FAILED tests/test_mesh_geometry.py::test_mixed_winding_is_reported_and_rewound - assert False

The assertion failing is on line 219-223, which checks that the warning message contains both "mixed vertex winding" and a specific pattern like "19 of 20".

The problem is in the logging message on lines 1274-1282 of mesh.py. The message includes mesh['n_tde'] at the end, but when the test runs, the logged message appears to not match the expected format.

Looking at the code flow, I notice:

1. On line 1277, it formats n_downward (the count of downward triangles)
2. Then it refers to mesh['n_tde'] in the format string
3. However, the test expects the pattern f"{mesh.n_tde - 1} of {mesh.n_tde}"

The issue is likely in the warning message format. The test creates a mesh with 20 triangles where the first one is reversed (reverse_first=True), making 19 CCW and 1 CW. When the warning is logged, it should say "1 of 20" (1 downward triangle out of 20 total), not "19 of 20".

Looking more carefully at line 1277 in the warning message:

f"{n_downward} of {mesh['n_tde']} triangles have downward"

This should produce "1 of 20", which matches the test expectation. But the test is checking:

f"{mesh.n_tde - 1} of {mesh.n_tde}" in message

This expects "19 of 20", not "1 of 20". The test logic seems inverted - it's looking for mesh.n_tde - 1 (19) but the actual message will have n_downward (1).

The fix: Update the logging message to include the count of CCW (upward) triangles instead of downward triangles, or update the test to match the actual message format.

Based on the PR description where it says the test was checking for "1 triangle downward, 1840 upward", the message should likely say "1 of 1841" (1 CW out of 1841 total). But the test mesh has 20 triangles total with 1 CW and 19 CCW, so the message should be "1 of 20".

The test expectation f"{mesh.n_tde - 1} of {mesh.n_tde}" expects "19 of 20", which would be the count of CCW triangles. The logging statement should report the CCW count instead of the CW count to match the test.

Solution: Change line 1277 in mesh.py from:

f"{n_downward} of {mesh['n_tde']} triangles have downward"

to:

f"{n_upward} of {mesh['n_tde']} triangles are counter-clockwise"

Or update it to match what the test expects more directly.

[jploveless]

I guess that makes sense, as I deleted the calculation of n_upward because it's not needed for the re-winding process.

If I try to add it back in, ruff automatically removes it upon commit because it's an unused variable.

I'm confused about copilot's reference to the logging on lines 1274+, as these no longer appear in the current commit. Is it using a previous commit as the basis for comparison in the test?

[brendanjmeade]

@jploveless You are right that Copilot was looking at an old version (typical). All I'm doing is repeating Copilot to you. I might suggest working with the little Copilot built into GitHub (pretty sure it's free) to get all tests to pass here!

---

The latest comment is pointing to the real issue: the Copilot diagnosis is stale and is comparing against older code, not the current PR state.

What the current code and test are actually doing:

• In celeri/mesh.py, the warning is emitted before the node swap.
• It computes n_downward from the original orientation and logs that count.
• The test in tests/test_mesh_geometry.py expects the warning to mention the downward count: mesh.n_tde - 1 of mesh.n_tde, i.e. “19 of 20” for a mesh with one upward triangle and 19 downward triangles.

So the correct fix is not to switch to n_upward; that would be the wrong metric for this warning and would trigger Ruff’s unused-variable complaint. The right fix is to keep the warning anchored to the pre-rewind downward count.

Suggested code pattern:

unit_z = mesh["nv"][:, 2] / np.linalg.norm(mesh["nv"], axis=1)
n_downward = int(np.sum(unit_z < -WINDING_TOLERANCE))
if n_downward > 0 and np.sum(unit_z > WINDING_TOLERANCE) > 0:
    logger.warning(
        f"Mesh {config.mesh_filename} has mixed vertex winding: "
        f"{n_downward} of {mesh['n_tde']} triangles have downward "
        "normals, so the sign of their dip-slip rates ..."
    )

And the key point for the PR discussion:

• The review comment about n_upward is based on an older revision of the file.
• The current code already matches the test expectation and the intended behavior: warn about the number of downward-wound triangles before rewinding them to CCW.

So my recommendation is:

• respond in the PR that the Copilot comment is stale and based on a previous revision,
• keep the current n_downward logic,
• optionally clarify in the code comment that the mixed-winding warning is intentionally about the pre-rewind downward count, not the post-rewind CCW count.

[jploveless]

Got it: Copilot notes that tests/test_mesh_geometry.py insists on very specific language in the warnings that are triggered by the node swapping. I changed from logger.info to logger.warning to categorize the logger text to agree, and I added the very specific text that the test requires. I also edited so that distinct warning messages are produced for the all-CW case vs. the mixed winding case. Tests are passing now 😮‍💨