Skip to content

Conversation

@egallmeier
Copy link
Collaborator

@egallmeier egallmeier commented Jun 9, 2025

Closes #1291

Overview

  • Fixed _populate_edge_face_distances and _construct_edge_face_distances in neighbors.py from using node coordinates to face coordinates
  • Added a simple test in test_neighbors.py to validate the _construct_edge_face_distances functionality.

Testing
Creates 5 face longitude and latitude pairs. The expected distances are:

  • Face 0 to Face 1: a quarter circle, pi/2 radians
  • Face 0 to Face 2: a quarter circle, pi/2 radians
  • Face 1 to Face 3: both at lat=90° (north pole), same point, distance = 0
  • Face 2 to Face 4: 3/8 of a circle

@egallmeier egallmeier self-assigned this Jun 9, 2025
@philipc2
Copy link
Member

philipc2 commented Jun 9, 2025

@egallmeier

Looks like one of gradient test is failing now, which is probably due to the change in the distance. The expected values we set before we most likely incorrect.

@philipc2 philipc2 changed the title Fix edge_face_distance functions in neighbors.py. Add unit test in test_neighbors.py. Closes #1291 Fix edge_face_distance functions in neighbors.py. Add unit test in test_neighbors.py Jun 10, 2025
Copy link
Member

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

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

It appears that one of the tests is still failing. 3 of the values seem to be mismatched.

@egallmeier
Copy link
Collaborator Author

@philipc2 are you referring to test_gradient.py::test_quad_hex?

@philipc2
Copy link
Member

@philipc2 are you referring to test_gradient.py::test_quad_hex?

Yeah, it seems to still be failing in the CI above.

@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Jun 13, 2025
@github-actions
Copy link

ASV Benchmarking

Benchmark Comparison Results

Benchmarks that have improved:

Change Before [5c74b0c] After [5c787c8] Ratio Benchmark (Parameter)
- 777M 346M 0.45 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
failed 755M n/a face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
- 380M 298M 0.79 mpas_ocean.Integrate.peakmem_integrate('480km')

Benchmarks that have stayed the same:

Change Before [5c74b0c] After [5c787c8] Ratio Benchmark (Parameter)
345M 345M 1.00 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
376M 376M 1.00 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
19.2±0.1ms 19.2±0.2ms 1.00 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
3.34±0.02ms 3.31±0.02ms 0.99 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
17.2±0.03ms 17.2±0.01ms 1.00 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
2.10±0ms 2.03±0.06ms 0.96 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
2.07±0.04s 2.07±0.03s 1.00 import.Imports.timeraw_import_uxarray
898±7μs 874±3μs 0.97 mpas_ocean.CheckNorm.time_check_norm('120km')
701±3μs 691±2μs 0.99 mpas_ocean.CheckNorm.time_check_norm('480km')
825±3ms 801±10ms 0.97 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('120km')
53.4±0.7ms 52.2±1ms 0.98 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('480km')
647±9μs 648±10μs 1.00 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('120km')
522±20μs 532±8μs 1.02 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('480km')
6.27±0.04ms 6.28±0.06ms 1.00 mpas_ocean.ConstructFaceLatLon.time_cartesian_averaging('120km')
4.72±0.04ms 4.69±0.03ms 0.99 mpas_ocean.ConstructFaceLatLon.time_cartesian_averaging('480km')
3.48±0.01s 3.50±0.01s 1.01 mpas_ocean.ConstructFaceLatLon.time_welzl('120km')
220±2ms 224±1ms 1.02 mpas_ocean.ConstructFaceLatLon.time_welzl('480km')
18.1±0.02ms 18.2±0.06ms 1.01 mpas_ocean.ConstructTreeStructures.time_ball_tree('120km')
923±20μs 891±10μs 0.96 mpas_ocean.ConstructTreeStructures.time_ball_tree('480km')
11.4±0.04ms 11.3±0.03ms 1.00 mpas_ocean.ConstructTreeStructures.time_kd_tree('120km')
1.10±0.03ms 1.10±0.01ms 1.00 mpas_ocean.ConstructTreeStructures.time_kd_tree('480km')
529±4ms 534±3ms 1.01 mpas_ocean.CrossSections.time_const_lat('120km', 1)
272±3ms 269±2ms 0.99 mpas_ocean.CrossSections.time_const_lat('120km', 2)
140±2ms 140±0.6ms 1.00 mpas_ocean.CrossSections.time_const_lat('120km', 4)
426±2ms 434±4ms 1.02 mpas_ocean.CrossSections.time_const_lat('480km', 1)
216±1ms 218±2ms 1.01 mpas_ocean.CrossSections.time_const_lat('480km', 2)
111±0.9ms 113±0.7ms 1.02 mpas_ocean.CrossSections.time_const_lat('480km', 4)
49.6±0.2ms 50.0±0.4ms 1.01 mpas_ocean.DualMesh.time_dual_mesh_construction('120km')
4.92±0.05ms 4.96±0.02ms 1.01 mpas_ocean.DualMesh.time_dual_mesh_construction('480km')
927±5ms 919±2ms 0.99 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', False)
54.1±0.5ms 54.2±0.3ms 1.00 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', True)
74.4±0.2ms 74.1±0.3ms 1.00 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', False)
5.12±0.06ms 5.10±0.08ms 1.00 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', True)
234M 235M 1.00 mpas_ocean.Gradient.peakmem_gradient('120km')
218M 219M 1.00 mpas_ocean.Gradient.peakmem_gradient('480km')
2.72±0ms 2.74±0.01ms 1.00 mpas_ocean.Gradient.time_gradient('120km')
336±7μs 336±8μs 1.00 mpas_ocean.Gradient.time_gradient('480km')
218±3μs 213±0.6μs 0.98 mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('120km')
126±2μs 126±0.7μs 1.00 mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('480km')
311M 311M 1.00 mpas_ocean.Integrate.peakmem_integrate('120km')
150±4ms 150±1ms 1.00 mpas_ocean.Integrate.time_integrate('120km')
18.0±0.08ms 18.3±0.4ms 1.02 mpas_ocean.Integrate.time_integrate('480km')
349±2ms 346±0.8ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'exclude')
353±7ms 349±0.9ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'include')
351±3ms 349±5ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'split')
22.7±0.1ms 23.0±0.2ms 1.01 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'exclude')
22.9±0.1ms 22.7±0.3ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'include')
23.1±0.2ms 22.6±0.2ms 0.98 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'split')
failed failed n/a mpas_ocean.PointInPolygon.time_face_search('120km')
failed failed n/a mpas_ocean.PointInPolygon.time_face_search('480km')
274±5ms 267±2ms 0.98 mpas_ocean.RemapDownsample.time_bilinear_remapping
293±2ms 291±1ms 0.99 mpas_ocean.RemapDownsample.time_inverse_distance_weighted_remapping
28.9±0.1ms 28.8±0.1ms 1.00 mpas_ocean.RemapDownsample.time_nearest_neighbor_remapping
1.38±0.01s 1.38±0.01s 1.00 mpas_ocean.RemapUpsample.time_bilinear_remapping
52.1±0.4ms 51.6±0.5ms 0.99 mpas_ocean.RemapUpsample.time_inverse_distance_weighted_remapping
26.5±0.3ms 26.3±0.4ms 1.00 mpas_ocean.RemapUpsample.time_nearest_neighbor_remapping
27.6±0.6ms 28.0±0.05ms 1.02 mpas_ocean.ZonalAverage.time_zonal_average('120km')
5.66±0.01ms 5.48±0.06ms 0.97 mpas_ocean.ZonalAverage.time_zonal_average('480km')
215M 217M 1.01 quad_hexagon.QuadHexagon.peakmem_open_dataset
214M 214M 1.00 quad_hexagon.QuadHexagon.peakmem_open_grid
7.52±0.02ms 7.56±0.03ms 1.01 quad_hexagon.QuadHexagon.time_open_dataset
6.52±0.02ms 6.57±0.02ms 1.01 quad_hexagon.QuadHexagon.time_open_grid

@philipc2 philipc2 removed the run-benchmark Run ASV benchmark workflow label Jun 13, 2025
Copy link
Member

@philipc2 philipc2 left a comment

Choose a reason for hiding this comment

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

Excellent work @egallmeier

Copy link
Collaborator

@kafitzgerald kafitzgerald left a comment

Choose a reason for hiding this comment

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

Looks great 🎉

@philipc2 philipc2 merged commit c24a153 into main Jun 13, 2025
21 checks passed
@erogluorhan erogluorhan deleted the fix-1291 branch September 26, 2025 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect edge-face arc-distance computation in neighbors.py

4 participants