Uh oh!
There was an error while loading. Please reload this page.
Remove a dead element-code check in MeshFile, and record why node Z is float64 - #53
Open
ecomodeller wants to merge 1 commit into
Open
Remove a dead element-code check in MeshFile, and record why node Z is float64#53ecomodeller wants to merge 1 commit into
ecomodeller wants to merge 1 commit into
Conversation
…at64 The element-code check could never do anything: if (elmtCode != 21) or (elmtCode != 25): pass The condition is true for every value — 21 fails the second test, 25 fails the first — so it needs `and`. That was harmless only because the body is `pass`, but the TODO beside it invited filling the body in, at which point the check would have rejected every valid mesh. ElementType is already derived per element from the corner count a few lines below, so the header's code needs no separate handling; the comment now says what elmtCode is and that nothing acts on it. Whether an unexpected element code should raise, warn, or continue to be ignored is a policy question, filed as an issue rather than decided here. The `# TODO or np.float32 ?` beside the Z array is also answerable: float64 is correct. A .mesh stores coordinates as text, so the file can carry more precision than float32 holds, and MeshBuilder.SetNodes narrows Z only because that path leads to a dfsu, where node Z occupies four bytes. Narrowing on read would drop digits the file actually contains, and mikeio reads MeshFile.Z straight into its mesh geometry, so the loss would be silent there. No behaviour change: both hunks are comments plus the removal of an if/pass.
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 freeto 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.
Two behaviour-preserving edits in
mikecore/MeshFile.py, both replacing a TODO with the answer it was asking for.1. Remove a dead element-code check. The block was
The condition is a tautology — true for every value, including 21 and 25; it needs
and. It is harmless only because the body ispass, so removing it is a runtime no-op. Removing rather than fixing the operator is deliberate: the TODO invites someone to fill the body in, and with the current condition that would reject every valid mesh. Whether an unexpected element code should raise, warn, or keep being ignored is a separate policy question, filed as #46.2. Record why node Z is
float64.# TODO or np.float32 ?is replaced by the reasoning for keepingfloat64: a.meshstores coordinates as text, so the file imposes no width and can carry more precision thanfloat32holds.MeshBuilder.SetNodesnarrows Z tofloat32because that is the path towards a dfsu, where node Z occupies four bytes — reading a file is not that path. Narrowing here would drop digits the file contains, and downstream consumers that stack the coordinates would upcast back tofloat64, hiding the loss.No expression, assignment or control-flow change; the suite counts are unchanged (
tests/test_eum.py::TestEUM::test_wrapperfails identically before and after — see #52).