Merge coincident pole and antimeridian nodes in structured grids - #1690
Merge coincident pole and antimeridian nodes in structured grids#1690rajeeja wants to merge 5 commits into
Conversation
Match nodes in Cartesian space rather than the lon/lat plane so pole and antimeridian nodes are recognized as the same point, and store the resulting polar faces as triangles instead of quads with a repeated corner.
|
Checked the docs and user guide for behavior changes: both datasets in |
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Thank you for your work on this! Requesting minor changes and clarifications. I will take a closer look at the tests after understanding things a bit better.
Can you clarify, what does this mean from your original comment?
- Now matches in Cartesian space, so nodes that are the same point on the sphere collapse: 899 nodes at 1 deg, 3599 at 0.25 deg.
I was also confused about the following, but I left an inline comment about it, feel free to respond there if that is easier:
- Merging leaves each polar quad with a repeated corner, so those faces are now stored as triangles padded with the fill value.
float32 input (e.g. real climate datasets) silently ran the whole xyz/tolerance pipeline at float32 precision, causing pole/antimeridian merges to fail or merge only partially.
Sevans711
left a comment
Sevans711
left a comment
There was a problem hiding this comment.
Looks almost ready to merge now, thank you for the changes! Left inline comments with minor style request and noting one typo. Other than that I think it looks good!
|
|
||
| # ``tol`` is an angle in degrees; on the unit sphere the matching radius is the | ||
| # chord subtended by that angle, so the threshold keeps its documented meaning. | ||
| chord_tol = 2.0 * np.sin(np.deg2rad(tol) / 2.0) |
There was a problem hiding this comment.
This conversion is now redundant with the changes to default tol above.
| def test_global_structured_grid_merges_poles_and_seam(): | ||
| """Nodes coincident on the sphere must be merged, even though their | ||
| (lon, lat) pairs differ. Regression test for issue #1689.""" | ||
| import numpy as np |
There was a problem hiding this comment.
minor but please move numpy import to top of file to match style with other testing suite files. Similar for import numpy call from test below.
3 participants
Closes #1689
_read_structured_gridmatched nodes in the lon/lat plane, so thenlon+1nodes at each pole and the two sides of the antimeridian were never merged.nlonas expected (36 at 10 deg, 360 at 1 deg) andV - E + F == 2confirms a closed sphere.