Conversation
and _check_intersection. Improves docstring, refactors slightly (but behavior should be the same) and removes the unused lonlat computations. Doesn't avoid allocating the `intersection_points` buffer because it's nontrivial to avoid that. (I separately tried it but my changes led to pytest suite failing and I'm not really sure why...) That buffer is only used for faces crossing the equator anyways, so it probably isn't worthwhile to worry too much about optimizing it...
now it avoids allocating tiny numpy arrays. (Nontrivial to optimize the call site (`barycentric_coordinates_cartesian`) though because the call site might return length 3 or length 4 numpy arrays, depending on inputs. Might be simple to optimize if forcing inputs to barycentric_coordinates_cartesian to be tuples, though, maybe?)
Sevans711
added
scalability
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
Benchmarks that have got worse:
|
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
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.
Closes #XXX
Overview
Optimizes numba routines in uxarray/grid/area.py to avoid constructing many tiny numpy arrays inside numba routines, as discussed in #1648.
This one was much more complicated to make progress on, compared to the other sub-issues already solved under 1648. The main reasons are: (1) its methods assign numpy arrays whose sizes depend on input variables' shapes, instead of being constant, so it's not trivial to convert to tuples; (2) there is less documentation and more "dead" code here that isn't doing anything, so it took some extra time to figure out what is going on; (3) there are more connections with methods defined in other parts of the code, such as arcs.py, bounds.py, bilinear.py, utils.py, either calling methods from here or being called by methods from here, and (4) call sites are often very deep (>5 clicks before getting to a top-level user-facing function) so it's trickier to track and understand the intended and actual use-cases for these functions sometimes.
Originally attempted more optimizations than this but had to backtrack when it make pytest suite fail in nontrivial ways that were difficult to debug.
Still very much a draft PR, mostly curious to see if these changes are enough to make any dent in ASV benchmarks, or if it might be better to focus more on some of those other files (arcs.py, bounds.py, etc) first. I suspect there might not be huge improvements just yet, because there are still quite a few tiny numpy arrays, and elsewhere I only really saw large speedups after finishing the cleanup to avoid all relevant numpy allocations.
PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: Claude for discussion and some code suggestions, plus GitHub Copilot's inline code suggestions