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:
|
Collaborator
|
Seems like this might be a sub-issue of #1789 2b? |
Collaborator
Author
Agreed, thank you for flagging this connection. I think #1789 2b is basically the same thing as implied by #1648 (though 1789 additionally does a good job of clearly pointing out many locations where this optimization could still help). My preference would be to continue tracking that via 1648 if that sounds reasonable? This PR fixes some of the geometry.py topics pointed out in that table, but it isn't yet seeing any noticeable speedups, so my plan is to keep it as draft for now, revisit after some of the other topics there get fixed, and then make a corresponding sub-issue of 1648 once this is ready for review. |
Collaborator
|
Yeah, that's fine, I'll just add a note in #1789 to track the sub-issues over there. I think the work can happen in parallel to the other sections, so that's actually kind of nice. |
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
2 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 #XXX
Overview
Optimizes numba routines in uxarray/grid/geometry.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