Cmd/esmf overflow bug - #1659
Draft
cmdupuis3 wants to merge 6 commits into
Draft
Conversation
The ESMF writer applied the 1-based index offset to every entry of face_node_connectivity, including padded slots, then encoded the result as int32. INT_FILL_VALUE + 1 (-2**63 + 1) truncates to 1 under that cast, so padding was written as a valid node index instead of the declared _FillValue of -1. Ragged grids silently gained vertices: a triangle padded to width 4 was written as a quad whose extra vertex was node 0. The numElementConn fallback, which counts entries != -1, was wrong for the same reason and reported every face at maximum width. The reader had the mirror-image defect. CF decoding turns the on-disk fill into NaN, and the code cast straight to INT_DTYPE and compared against INT_FILL_VALUE. That comparison only holds where NaN casts to INT64_MIN; on arm64 it casts to 0, so padding decoded to -1 -- a negative index that silently wraps to the last node rather than raising. Mask the padding explicitly on both sides. Existing ESMF fixtures are all pure-quad meshes with no padding, which is why the round-trip test never exercised this.
_encode_exodus searched for padding with `row == -1`, but connectivity padding is stored as INT_FILL_VALUE. The comparison never matched, so every face was treated as full width: mixed meshes were written as a single block typed for the widest face, the per-block element counts were wrong, and the padding itself was written out as a node index of INT_FILL_VALUE + 1. uxarray's own reader happened to invert that -- Exodus connectivity is int64, so the writer's +1 and the reader's -1 cancel exactly at INT_FILL_VALUE -- which is why the round-trip test passed. The emitted file is still not valid Exodus for any other consumer. Match on INT_FILL_VALUE so faces are grouped into correctly typed blocks. Because Exodus blocks are homogeneous, that regroups a mixed mesh, so also write elem_num_map recording each element's original position and have _read_exodus invert it when it is a true permutation. Without that the faces come back reordered and any face-centered data silently misaligns -- a worse failure than the one being fixed.
_encode_scrip wrote NaN into grid_corner_lat/lon for every padded slot. SCRIP has no fill value for corners, so on read-back those NaNs dedupe into a real node: a grid with one padded triangle came back with an extra node whose coordinates are NaN, inflating n_node and feeding NaN into every downstream geometry calculation. Write the SCRIP-conventional degenerate polygon instead, repeating the face's last valid corner into the padded slots. Node count and node coordinates now round-trip correctly. Note this is not an exact connectivity round-trip: the padded face comes back as a degenerate quad with a repeated vertex rather than a triangle, which is what SCRIP can express. Collapsing trailing duplicate corners back to INT_FILL_VALUE would need a reader change affecting every existing SCRIP file, including legitimately degenerate ones.
Every writable format shared the same untested path: connectivity padding on a mesh with mixed face sizes. All the ESMF, Exodus, and SCRIP fixtures are uniform meshes that pad nothing, so the encoders' fill-value handling was never exercised and three separate corruption bugs went unnoticed. Add TestIOWriteRoundTrip in test_io_common.py, parametrized over the WRITABLE_FORMATS list that was already defined there but unused. One ragged fixture (a quad and two triangles) now covers all four writers: node count and coordinates must survive, no negative leftovers may remain in the connectivity, and the index-based formats must restore it verbatim including face order. SCRIP is held to a weaker contract. It stores corner coordinates rather than indices, so its reader renumbers nodes and a short face round-trips as a degenerate polygon; _face_geometry compares faces by coordinate instead of by index so it can still be checked. Round-trip assertions alone miss the Exodus bug: connectivity there is int64, so the writer's +1 and the reader's -1 cancel exactly at INT_FILL_VALUE and the grid reloads intact from a file holding an index no other reader could use. test_ragged_grid_encodes_usable_indices inspects the encoded output directly to catch it. Consolidation: test_esmf_round_trip_consistency was 68 lines of manual file handling covering one format on a uniform mesh, now subsumed by the parametrized version and reduced to a structural check. The empty test_encode_exodus placeholder is filled in, and the Exodus block splitting and elem_num_map get their own test alongside it. Verified by reverting all three fixes: 6 of these fail, covering each bug.
Collaborator
Author
|
pre-commit.ci autofix |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1658
Overview
Several related bugs in how fill values are used in IO routines. Original case is the first one, where a downcast of
INT_FILL_VALUEresults in valid indices, resulting in spurious extra nodes and incorrect polygons.uxarray/io/_esmf.py_encode_esmf+1applied to fill slots, then int32 encoding truncates-2**63+1→1. Padding written as node 0.uxarray/io/_esmf.py_read_esmfNaN → INT64_MIN; arm64 gives0, so padding decodes to-1— a negative index that wraps.uxarray/io/_exodus.py_encode_exodus== -1, but padding isINT_FILL_VALUE. Never fired → one max-width block, wrong element types,INT_FILL_VALUE+1written as an index.uxarray/io/_exodus.py_encode_exoduselem_num_map; face-centered data silently misaligns.uxarray/io/_scrip.py_encode_scripNaNwritten to padded corners; reader deduplicates it into a real node.n_node5→6, NaN coordinates.PR Checklist
General
Testing & Benchmarking
Documentation
docs/api.rst_)AI Disclosure
Claude Opus 5, for bug search and generated code.
AI Usage: