Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimized Geometry Functions using Numba #1072

Merged
merged 35 commits into from
Nov 20, 2024
Merged

Optimized Geometry Functions using Numba #1072

merged 35 commits into from
Nov 20, 2024

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Nov 10, 2024 •
edited
Loading

Closes #936

Overview

  • Rewrites the following functions to avoid needing to convert between Cartesian and Spherical Coordinates
    • point_within_gca()
    • extreme_gca_latitude()
    • gca_gca_intersection()
  • Added compatibility functions with less-strict array and parameter requirements to use for testing.
    • _point_within_gca_cartesian()
    • _extreme_gca_latitude_cartesian()
    • _gca_gca_intersection_cartesian()
  • Removed the use of the pyfma package.
  • The bounds for each face is computed in parallel using Numba
    image

@philipc2
Copy link
Member Author

philipc2 commented Nov 10, 2024 •
edited
Loading

@hongyuchen1030

I still need to finish adjusting all the tests and ensure that everything is passing, but I wanted to quickly updated you on some initial timings.

  • 30km (~40,000 mostly hexagonal faces): 4-10 seconds including the initial Numba compilation.

This was without any parallelization. Since the bounding box for each face can be computed interpedently, my next step will be to parallelize the loop that calls _populate_face_latlon_bound

@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Nov 10, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2024 •
edited
Loading

ASV Benchmarking

Benchmark Comparison Results

Benchmarks that have improved:

Change Before [fd757b0] After [879b432] Ratio Benchmark (Parameter)
- 1.57±0s 22.8±0.1ms 0.01 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
- 226±2ms 7.78±0.09ms 0.03 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
- 1.98±0.01s 49.3±0.2ms 0.02 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
- 7.83±0.1ms 3.70±0.02ms 0.47 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
- 467M 373M 0.8 mpas_ocean.Integrate.peakmem_integrate('480km')

Benchmarks that have stayed the same:

Change Before [fd757b0] After [879b432] Ratio Benchmark (Parameter)
400M 420M 1.05 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
443M 412M 0.93 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
2.82±0.01s 2.79±0.01s 0.99 import.Imports.timeraw_import_uxarray
646±9μs 652±10μs 1.01 mpas_ocean.CheckNorm.time_check_norm('120km')
423±4μs 443±7μs 1.05 mpas_ocean.CheckNorm.time_check_norm('480km')
645±7ms 642±9ms 1.00 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('120km')
41.0±0.7ms 40.9±0.7ms 1.00 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('480km')
1.73±0.05ms 1.74±0.1ms 1.00 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('120km')
6.04±0.04ms 5.84±0.04ms 0.97 mpas_ocean.ConstructFaceLatLon.time_cartesian_averaging('120km')
3.52±0.03ms 3.52±0.06ms 1.00 mpas_ocean.ConstructFaceLatLon.time_cartesian_averaging('480km')
3.54±0.01s 3.51±0.01s 0.99 mpas_ocean.ConstructFaceLatLon.time_welzl('120km')
227±2ms 225±3ms 0.99 mpas_ocean.ConstructFaceLatLon.time_welzl('480km')
263±1ns 265±1ns 1.01 mpas_ocean.ConstructTreeStructures.time_ball_tree('480km')
262±4ns 273±9ns 1.04 mpas_ocean.ConstructTreeStructures.time_kd_tree('480km')
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('120km', 1)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('120km', 2)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('120km', 4)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('120km', 8)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('480km', 1)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('480km', 2)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('480km', 4)
failed failed n/a mpas_ocean.CrossSections.time_constant_lat_fast('480km', 8)
122±1ms 123±1ms 1.01 mpas_ocean.DualMesh.time_dual_mesh_construction('120km')
8.91±0.09ms 8.68±0.2ms 0.97 mpas_ocean.DualMesh.time_dual_mesh_construction('480km')
1.02±0s 1.03±0s 1.01 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', False)
52.1±0.6ms 51.9±0.6ms 1.00 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', True)
76.6±0.2ms 76.8±0.3ms 1.00 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', False)
5.34±0.1ms 5.25±0.05ms 0.98 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', True)
321M 319M 0.99 mpas_ocean.Gradient.peakmem_gradient('120km')
296M 296M 1.00 mpas_ocean.Gradient.peakmem_gradient('480km')
2.76±0.01ms 2.75±0.03ms 1.00 mpas_ocean.Gradient.time_gradient('120km')
306±0.6μs 307±0.9μs 1.00 mpas_ocean.Gradient.time_gradient('480km')
failed failed n/a mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('120km')
failed failed n/a mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('480km')
389M 389M 1.00 mpas_ocean.Integrate.peakmem_integrate('120km')
174±1ms 173±0.8ms 0.99 mpas_ocean.Integrate.time_integrate('120km')
11.9±0.07ms 12.0±0.06ms 1.01 mpas_ocean.Integrate.time_integrate('480km')
336±4ms 333±4ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'exclude')
335±4ms 338±6ms 1.01 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'include')
337±4ms 338±3ms 1.00 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'split')
22.9±0.9ms 21.9±0.4ms 0.96 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'exclude')
22.3±0.2ms 21.9±0.4ms 0.98 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'include')
22.5±0.6ms 21.9±0.5ms 0.98 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'split')
55.1±0.3ms 55.4±0.2ms 1.00 mpas_ocean.RemapDownsample.time_inverse_distance_weighted_remapping
44.9±0.3ms 45.2±0.2ms 1.01 mpas_ocean.RemapDownsample.time_nearest_neighbor_remapping
358±2ms 362±2ms 1.01 mpas_ocean.RemapUpsample.time_inverse_distance_weighted_remapping
264±1ms 264±0.4ms 1.00 mpas_ocean.RemapUpsample.time_nearest_neighbor_remapping
294M 292M 0.99 quad_hexagon.QuadHexagon.peakmem_open_dataset
291M 291M 1.00 quad_hexagon.QuadHexagon.peakmem_open_grid
6.22±0.1ms 6.32±0.06ms 1.02 quad_hexagon.QuadHexagon.time_open_dataset
5.36±0.03ms 5.32±0.03ms 0.99 quad_hexagon.QuadHexagon.time_open_grid

Benchmarks that have got worse:

Change Before [fd757b0] After [879b432] Ratio Benchmark (Parameter)
+ 375M 417M 1.11 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
+ 375M 417M 1.11 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
+ 500±10μs 562±6μs 1.12 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('480km')
+ 1.08±0μs 1.20±0.01μs 1.1 mpas_ocean.ConstructTreeStructures.time_ball_tree('120km')
+ 717±5ns 795±5ns 1.11 mpas_ocean.ConstructTreeStructures.time_kd_tree('120km')

@philipc2 philipc2 removed the run-benchmark Run ASV benchmark workflow label Nov 10, 2024
@philipc2 philipc2 self-assigned this Nov 10, 2024
@philipc2 philipc2 added this to the Scalability & Performance milestone Nov 10, 2024
@philipc2 philipc2 added the scalability Related to scalability & performance efforts label Nov 10, 2024
@philipc2 philipc2 added run-benchmark Run ASV benchmark workflow and removed run-benchmark Run ASV benchmark workflow labels Nov 10, 2024
@philipc2 philipc2 added run-benchmark Run ASV benchmark workflow and removed run-benchmark Run ASV benchmark workflow labels Nov 11, 2024
@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Nov 11, 2024
@philipc2 philipc2 removed the run-benchmark Run ASV benchmark workflow label Nov 12, 2024
@hongyuchen1030
Copy link
Contributor

@hongyuchen1030

I might scrap the previous methods for computing the intersections and exclusively support the methods that use the bounding box now that the performance has significantly improved.

sounds great!

@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Nov 18, 2024
@philipc2 philipc2 removed the run-benchmark Run ASV benchmark workflow label Nov 18, 2024
hongyuchen1030
hongyuchen1030 requested changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@hongyuchen1030 hongyuchen1030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you remove all face_latlon_bounds usages in this PR?

Please prioritize merging the face_latlon_bounds here. You can set the use_spherical_bounds by default as false, but please try to at least keep the implementation in this PR

@philipc2
Copy link
Member Author

Did you remove all face_latlon_bounds usages in this PR?

Please prioritize merging the face_latlon_bounds here. You can set the use_spherical_bounds by default as false, but please try to at least keep the implementation in this PR

I did the opposite. I removed the usages of the naive implementation. All constant longitude and constant latitude queries now use the face_latlon_bounds.

There is no longer a use_spherical_bounds parameter. I hope this clarifies.

@hongyuchen1030
Copy link
Contributor

Did you remove all face_latlon_bounds usages in this PR?
Please prioritize merging the face_latlon_bounds here. You can set the use_spherical_bounds by default as false, but please try to at least keep the implementation in this PR

I did the opposite. I removed the usages of the naive implementation. All constant longitude and constant latitude queries now use the face_latlon_bounds.

There is no longer a use_spherical_bounds parameter. I hope this clarifies.

Ohh I see. I guess I was misled by those “No implementation warning ” for use_spherical_bounds . Is there any reason we kept those?

@philipc2 philipc2 requested a review from rytam2 November 19, 2024 18:40
@philipc2
Copy link
Member Author

Did you remove all face_latlon_bounds usages in this PR?
Please prioritize merging the face_latlon_bounds here. You can set the use_spherical_bounds by default as false, but please try to at least keep the implementation in this PR

I did the opposite. I removed the usages of the naive implementation. All constant longitude and constant latitude queries now use the face_latlon_bounds.
There is no longer a use_spherical_bounds parameter. I hope this clarifies.

Ohh I see. I guess I was misled by those “No implementation warning ” for use_spherical_bounds . Is there any reason we kept those?

Those are the edge queries. Right now I am only using the face_bounds to determine if a face intersects with a line of longitude or latitude.

hongyuchen1030
hongyuchen1030 approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@hongyuchen1030 hongyuchen1030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

@philipc2 philipc2 linked an issue Nov 20, 2024 that may be closed by this pull request
Use Numba for geometry helper functions #1082
Closed
@philipc2 philipc2 merged commit b6b1eb8 into main Nov 20, 2024
20 checks passed
This was referenced Nov 27, 2024
Open
Merged
Closed
@erogluorhan erogluorhan deleted the philipc2/bounds-numba branch February 5, 2025 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongyuchen1030 hongyuchen1030 hongyuchen1030 approved these changes

@aaronzedwick aaronzedwick Awaiting requested review from aaronzedwick

@amberchen122 amberchen122 Awaiting requested review from amberchen122

@erogluorhan erogluorhan Awaiting requested review from erogluorhan

@rytam2 rytam2 Awaiting requested review from rytam2

Assignees

@philipc2 philipc2

Labels
scalability Related to scalability & performance efforts
Projects
None yet
Milestone
Scalability & Performance
2 participants
@philipc2 @hongyuchen1030