Fix left-handed MAPAXES placing the model in the wrong location - #60
Merged
Conversation
A MAPAXES keyword may define a left-handed pair of axes. Applying it as specified mirrors the grid's orientation, giving every element a negative Jacobian. The reader compensated by negating the x-axis vector before the transform, which does restore positive Jacobians -- but by mirroring the model into the wrong place. For the grid reported in issue #37 (MAPAXES -10000 9000 -10000 10000 -9000 10000, local extent 0..20000), Petrel reports bounds of x: -10000..10000. The reader produced x: -30000..-10000: right shape, wrong place, and with healthy looking Jacobians giving no hint that anything was wrong. Handedness is a property of the node ordering, not the node positions, so fix it in the ordering: apply the transform exactly as MAPAXES specifies, then reverse the j index order, which relabels cells without moving them. This is the same mechanism the reader already used for grids whose COORD is in decreasing order, which is why that path always produced correct geometry -- it just ran before the transform and so never saw an inversion introduced by MAPAXES. The new orientation test uses the determinant of the lateral (i, j) direction vectors rather than per-axis monotonicity, so it stays correct when a rotation leaves x varying along j rather than along i. The pre-existing monotonicity checks are kept as they are. They also canonicalise the index order rather than only the handedness: a grid with both axes decreasing is already right-handed but still needs both indices reversed to match the conventional ordering. Adds mapaxes_lefthanded.grdecl covering the exact case from the issue. Its four PORO values are distinct so the gold pins which cell landed where, not just the bounding box -- a mirrored copy of that symmetric grid has an identical bounding box. Verified that this test fails on master, with all four cells misplaced. gold/simple_cube_mapaxes.e is regenerated: its MAPAXES is also left-handed (a reflection swapping x and y), so the old gold encoded the mirrored output. The replacement was checked against the analytic transform -- every cell centroid at (local_y, local_x) with its properties -- rather than blessed from program output. Also drops the flip_x/flip_y locals, which were assigned but never read, and extracts the shared axis-reversal into _reverseGridAxis. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
A MAPAXES keyword may define a left-handed pair of axes. Applying it as specified mirrors the grid's orientation, giving every element a negative Jacobian. The reader compensated by negating the x-axis vector before the transform, which does restore positive Jacobians -- but by mirroring the model into the wrong place.
For the grid reported in issue #37 (MAPAXES -10000 9000 -10000 10000 -9000 10000, local extent 0..20000), Petrel reports bounds of x: -10000..10000. The reader produced x: -30000..-10000: right shape, wrong place, and with healthy looking Jacobians giving no hint that anything was wrong.
Handedness is a property of the node ordering, not the node positions, so fix it in the ordering: apply the transform exactly as MAPAXES specifies, then reverse the j index order, which relabels cells without moving them. This is the same mechanism the reader already used for grids whose COORD is in decreasing order, which is why that path always produced correct geometry -- it just ran before the transform and so never saw an inversion introduced by MAPAXES.
The new orientation test uses the determinant of the lateral (i, j) direction vectors rather than per-axis monotonicity, so it stays correct when a rotation leaves x varying along j rather than along i.
The pre-existing monotonicity checks are kept as they are. They also canonicalise the index order rather than only the handedness: a grid with both axes decreasing is already right-handed but still needs both indices reversed to match the conventional ordering.
Adds mapaxes_lefthanded.grdecl covering the exact case from the issue. Its four PORO values are distinct so the gold pins which cell landed where, not just the bounding box -- a mirrored copy of that symmetric grid has an identical bounding box. Verified that this test fails on master, with all four cells misplaced.
gold/simple_cube_mapaxes.e is regenerated: its MAPAXES is also left-handed (a reflection swapping x and y), so the old gold encoded the mirrored output. The replacement was checked against the analytic transform -- every cell centroid at (local_y, local_x) with its properties -- rather than blessed from program output.
Also drops the flip_x/flip_y locals, which were assigned but never read, and extracts the shared axis-reversal into _reverseGridAxis.