Repository navigation
Conversation
Checked an example raster is equivalent (via casting to xr.DataArray then checking .equals) before and after this commit.
renames old _face_contains_point to _face_contains_point_from_edges, for now. Next, delete it entirely, and rewrite call sites appropriately to use the new _point_in_face syntax instead. Confirmed that a raster is exactly the same, before and after these changes. Only a small speedup so far (~4.70s to ~4.45s), but there are still more optimizations to do!
Something in test_cross_sections.py is breaking now (numba python types compiler errors) but all other tests pass.
(there are no more references to it; forgot to remove it during previous commit.)
and _point_in_face, by avoiding tiny numpy arrays. Still only a small speedup for to_raster though... ~4.30s compared to ~4.45s from 594739b (and ~4.70s on main). Confirmed that the pytest suite passes locally, and that a produced raster is exactly the same as on main, via .equals().
Although, this actually provides no noticeable speed improvement to to_raster()...
Sevans711
added
scalability
ASV BenchmarkingBenchmark Comparison ResultsBenchmarks that have improved:
Benchmarks that have stayed the same:
|
Sevans711
marked this pull request as ready for review
October 6, 2026 15:12
Collaborator
|
For the benchmarks, can you trim them down a bit? I think we probably only need two scales for the timing and peakmem benchmarks, atm there are 7-8. As long as we can see the scaling difference we expect, we don't really need the rest. |
Collaborator
Author
Definitely, that makes sense. I trimmed it to just 3 parameter pairings: (n_face, area_ratio) = (1e4, 10), (1e4, 100), (1e5, 100). Comparing (1e4, 10) to (1e4, 100) shows scaling with resolution variability, so we can learn lessons like "10x more resolution variability increases costs by roughly 3x." Comparing (1e4, 100) to (1e5, 100) shows scaling with number of faces, so we can learn lessons like "10x more faces increases costs by only roughly 10%. The other parameter pairings help build a clearer picture across a wider parameter space, but it does make sense to me that we should pick a minimal set for the benchmark suite. I didn't trim down further to 2 pairings because that would make it impossible to separate the "resolution variability" scaling from the "number of faces" scaling. |
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 #1796 (sub-issue of #1648)
Closes #1358
Overview
Speeds up to_raster() by refactoring to avoid creating tiny numpy arrays inside numba routines (see #1648 and other sub-issues of it for more details). As mentioned in #1796 (comment), to_raster() was incredibly slow when applied to grids with varying resolution, causing many candidate faces per point, and calling relevant numba methods from
point_in_face.pymany times.This work also happens to close #1358; the NumbaPerformanceWarning mentioned there is no longer being raised after these refactors.
The refactors here aren't just purely swapping numpy arrays for tuples, and numpy operations for
uxarray.utils.numba_math.pyoperations. The biggest reason for this is that simply replacing those things in-place wouldn't be sufficient get around the issue with_get_cartesian_face_edge_nodes(), because n_edges can vary from face to face. This PR refactors to entirely avoid using that function anywhere in to_raster's call stack; the old_face_contains_point(face_edges, point)has been replaced by a new_point_in_face(point, nodes_idx, node_x, node_y, node_z). Other parts of the code which used_face_contains_pointhave been updated accordingly, and the new_point_in_face_from_gridprovides a convenient way to call this (especially for tests).Overall, the changes follow the descriptions laid out by #1796. Nothing should be too surprising in here. Added benchmarks to test to_raster() across a variety of grid sizes and resolution-variabilities (ratio between largest-area face and smallest-area face).
Performance notes from ASV benchmarks:
mainbecause they took longer than 180 seconds, which is what I chose for the timeout for these cases.main, due to timeout (it takes longer because of thewith numba_threads(1):block).mainand on the branch, which isn't too surprising; the main goal here was to speed up processing, not to reduce memory usage. Though, my guess is that the peakmem performance might be improved by addressing Speed up to_raster() by pruning candidate faces by their own radii, before checking point_in_face #1807.Sidenote: the benchmarks suite takes roughly 70 mins to run on this branch due to the cases which currently timeout onEDIT: no longer relevant, after commenting out many of the parameters; took roughly 30 mins to run the suite here, which is comparable to how long it took in #1710.main. However, the longest benchmark within this commit itself is still less than 15 seconds, so once this becomes the new baseline, I wouldn't expect a huge increase in benchmark suite runtime in the future.PR Checklist
General
Testing & Benchmarking
Documentation and Examples
docs/api.rst; internal (private) function names start with an underscore (_)AI Disclosure
AI Usage: Claude conversations (especially for building benchmarks) and GitHub Copilot's inline code suggestions