Implements Grid.compute_face_node_angles() - #1672
Merged
Merged
Conversation
This provides an initial implementation which seems to be working. See #1566 TODO: - support assume_convex=False - type-hint possible UxDataArray return type? - tests - examples
removes "geometry" (always use spherical) removes "assume_convex" (always assume convex) Also, improves type-hinting for compute_face_node_angles(). Also, formats using pre-commit ruff formatting.
use tuples instead of tiny arrays in numba! Cleans up docstring in grid.py (forgot to change in previous commit).
(it was making a type error in numba when trying to run the method)
compute_face_node_angles() seems to be really fast, so including a cache option in initial implementation isn't worthwhile.
Sevans711
added
new feature
Sevans711
requested review from
cmdupuis3,
erogluorhan,
hongyuchen1030 and
rajeeja
August 12, 2026 15:09
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
|
Collaborator
Author
|
Looks like this PR is having a nice side effect: the optimization to (The chain of calls is deep, so I'm putting it here for reference: Ignoring the apparent peakmem improvements (see #1605). Most importantly, none of the benchmarks show performance degradation, which helps demonstrate that the |
erogluorhan
approved these changes
Aug 14, 2026
erogluorhan
left a comment
erogluorhan
left a comment
Member
There was a problem hiding this comment.
This looks great to me! Thanks for all the invaluable discussion throughout this!
erogluorhan
reviewed
Aug 14, 2026
cmdupuis3
requested changes
Aug 18, 2026
cmdupuis3
approved these changes
Aug 18, 2026
cmdupuis3
left a comment
cmdupuis3
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, tests pass and the tests and numba parallelization look reasonable. The vector arithmetic will probably need to sync up with other incoming PRs in the future though
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
3 participants
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 #1566
Overview
Implements new function:
Grid.compute_face_node_angles(), which computes the internal angles at each node, for each face in a grid. Intentionally excludes many options initially suggested in #1566, as the discussion there revealed:size=n_faces*n_max_face_nodes), so it doesn't really make sense to include a caching option right now.Also adds corresponding tests, checking that the angle computations give correct values in a roughly 30,60,90 degree triangle and a hexagon grid. Also checks nan handling in a grid with both pentagons and hexagons (result should be nan wherever
n_max_face_nodesdimension index is larger thann_nodes_per_face). For all three cases, checks that the generalized spherical excess formula holds, by comparing toGrid.face_areasresults.Within scope but possibly unexpected change included this PR: refactored
_small_angle_of_2_vectorsto improve efficiency when inputs are not numpy arrays, and rewrote docstring accordingly. (See also: #1648.) This was necessary to improve efficiency forcompute_face_node_angles()without inlining all of the_small_angle_of_2_vectorslogic. This is unlikely to affect efficiency elsewhere in the code, because other regions of the code seem to always construct tiny numpy arrays before calling_small_angle_of_2_vectors. Running ASV benchmarks here to check, just in case (but it would be acceptable if benchmarks show no improvements).Also related (may help visualize face node angle results, if implemented): #1578
Expected Usage
PR Checklist
General
Testing & Benchmarking
Documentation
docs/api.rst_)AI Disclosure
AI Usage: discussions with Claude and ChatGPT, inline code suggestions from GitHub Copilot. Also (not in this PR, but contributed to decisions, see original issue for details) used Claude to make a numpy-only method for efficiency comparisons, and other people used Claude to iterate and run tests for more efficiency improvements.