Proposed new feature or change:
UXarray documentation should clearly clarify that the code assumes all faces are convex (all angles less than 180 degrees), and may silently fail or give incorrect results otherwise. This fundamental assumption should be clarified somewhere easy to see (e.g., on the readme). See also: #1597.
A better long-term solution might be to instead add a check for "are all faces convex?" and crash or at least produce a warning if false. However, there does not seem to be wide enough interest at the moment to prioritize developing that check right now.
Copying all relevant parts of the discussion from #1566 below, for reference:
(link to comment) Yes, we already implicitly assume convex/star-shaped faces: face_areas triangulates each face as a fan from node 0 (exact for convex, silently wrong for non-convex - it won't error), and the barycentric helpers in geometry.py/neighbors.py are documented convex-only. So a convexity check is a good idea - I'd make it a shared is_face_convex mask so face_areas/remapping can reuse it, and just fail loudly (with a clear message) in face_node_angles for now.
(link to comment) most geoscientific grid cells are generally “convex enough” and do not cause problems for the algorithms we use. In computational geometry, a more serious concern would be whether a face contains a hole—for example, a donut-shaped polygon. To my knowledge, geoscientific grids do not introduce this type of geometry.
(link to comment) To my knowledge, none of the model outputs we are supporting might come natively with concave faces, and our implicit assumption of convex faces only grids seems to have worked so far. If that is the case, we might not need an explicit convex face check.
(link to comment) Some years ago, if you were using refined grids on spectral elements using GLL nodes with the old "CUBIT-style" refinement, you could get non-convex faces for the physics cells (which were centered on the shared nodes and therefore could have something like 6-10 sides). I used to have many "hack" scripts to massage SCRIP grids to handle that during remapping accordingly.
I wonder if any coastal ocean/surge models would be problematic here. Many of them use finite elements on triangular meshes (which wouldn't be a problem here) but I'm not sure if there are more complex polygons applied.
(link to comment) I don't think we need the "convex" check. Given how hard it is to determine a face is actually "convex" on the sphere and the fact that most of our face are not very ill-conditioned. Our algorithms should cover all convex cases and mildly concave faces as well.
Marking this as "bug" in addition to "documentation" because uxarray currently silently handles non-convex cases incorrectly, which should be considered unexpected behavior, at least until the documentation clearly says that all faces are assumed to be convex.
Proposed new feature or change:
UXarray documentation should clearly clarify that the code assumes all faces are convex (all angles less than 180 degrees), and may silently fail or give incorrect results otherwise. This fundamental assumption should be clarified somewhere easy to see (e.g., on the readme). See also: #1597.
A better long-term solution might be to instead add a check for "are all faces convex?" and crash or at least produce a warning if false. However, there does not seem to be wide enough interest at the moment to prioritize developing that check right now.
Copying all relevant parts of the discussion from #1566 below, for reference:
Marking this as "bug" in addition to "documentation" because uxarray currently silently handles non-convex cases incorrectly, which should be considered unexpected behavior, at least until the documentation clearly says that all faces are assumed to be convex.