Skip to content

Fix structured from_xarray n_face mapping (#1410) - #1569

Merged
rajeeja merged 3 commits into
mainfrom
rajeeja/fix_1410
Jul 17, 2026
Merged

Fix structured from_xarray n_face mapping (#1410)#1569
rajeeja merged 3 commits into
mainfrom
rajeeja/fix_1410

Conversation

@rajeeja

@rajeeja rajeeja commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Fixes #1410 and the three bugs behind it:

  1. from_structured(lon=, lat=) used lowercase "structured", so the structured-flatten branch never ran.
  2. It passed source_dims_dict=None, so (lon, lat) → n_face was never recorded.
  3. from_dataset hard-coded source_dims_dict = {}, dropping any passed mapping.

rajeeja added 2 commits July 13, 2026 15:50
Grid.from_structured(lon=, lat=) recorded no source-dim mapping and used a
lowercase "structured" spec, so UxDataset.from_xarray could not flatten
(lon, lat) data variables onto n_face, leaving them unmapped and breaking
subsetting. Capture the lon/lat dim names, use the canonical "Structured"
spec, and honor a passed source_dims_dict in from_dataset. Closes #1410.
@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@erogluorhan erogluorhan added the run-benchmark Run ASV benchmark workflow label Jul 15, 2026
@github-actions

Copy link
Copy Markdown

ASV Benchmarking

Benchmark Comparison Results

Benchmarks that have improved:

Change Before [736bc03] After [f3e9ca5] Ratio Benchmark (Parameter)
- 579M 391M 0.67 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
- 700M 390M 0.56 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
- 498M 384M 0.77 mpas_ocean.Gradient.peakmem_gradient('480km')

Benchmarks that have stayed the same:

Change Before [736bc03] After [f3e9ca5] Ratio Benchmark (Parameter)
9.88±0.1μs 9.81±0.06μs 0.99 bench_connectivity.Connectivity.time_edge_face('120km')
10.2±0.1μs 10.1±0.1μs 0.99 bench_connectivity.Connectivity.time_edge_face('480km')
9.95±0.04μs 9.90±0.1μs 0.99 bench_connectivity.Connectivity.time_edge_node('120km')
10.1±0.06μs 10.3±0.1μs 1.02 bench_connectivity.Connectivity.time_edge_node('480km')
9.88±0.06μs 9.94±0.1μs 1.01 bench_connectivity.Connectivity.time_face_edge('120km')
10.2±0.09μs 10.6±0.06μs 1.03 bench_connectivity.Connectivity.time_face_edge('480km')
9.91±0.08μs 9.89±0.05μs 1 bench_connectivity.Connectivity.time_face_face('120km')
10.1±0.1μs 10.1±0.06μs 1 bench_connectivity.Connectivity.time_face_face('480km')
20.8±0.2μs 20.0±0.09μs 0.96 bench_connectivity.Connectivity.time_face_node('120km')
20.3±0.2μs 21.2±0.8μs 1.04 bench_connectivity.Connectivity.time_face_node('480km')
9.92±0.04μs 9.85±0.02μs 0.99 bench_connectivity.Connectivity.time_node_edge('120km')
10.4±0.4μs 10.1±0.1μs 0.97 bench_connectivity.Connectivity.time_node_edge('480km')
9.96±0.2μs 9.75±0.07μs 0.98 bench_connectivity.Connectivity.time_node_face('120km')
10.3±0.09μs 10.1±0.03μs 0.98 bench_connectivity.Connectivity.time_node_face('480km')
389M 389M 1 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
421M 419M 0.99 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
12.8±0.04ms 12.8±0.02ms 1 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
3.27±0.02ms 3.29±0.08ms 1 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
17.4±0.1ms 17.4±0.08ms 1 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
1.99±0.04ms 1.99±0.04ms 1 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
998±8ms 990±7ms 0.99 import.Imports.timeraw_import_uxarray
906±30ns 904±4ns 1 mpas_ocean.CheckNorm.time_check_norm('120km')
844±3ns 855±2ns 1.01 mpas_ocean.CheckNorm.time_check_norm('480km')
721±2ms 727±20ms 1.01 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('120km')
45.1±0.2ms 45.0±0.3ms 1 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('480km')
621±7μs 623±6μs 1 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('120km')
556±7μs 553±7μs 1 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('480km')
5.04±0.03ms 5.06±0.01ms 1.01 mpas_ocean.ConstructFaceLatLon.time_cartesian_averaging('120km')
3.55±0.02ms 3.52±0.02ms 0.99 mpas_ocean.ConstructFaceLatLon.time_cartesian_averaging('480km')
3.30±0.03s 3.33±0.02s 1.01 mpas_ocean.ConstructFaceLatLon.time_welzl('120km')
215±2ms 215±0.6ms 1 mpas_ocean.ConstructFaceLatLon.time_welzl('480km')
19.7±0.03ms 19.8±0.1ms 1.01 mpas_ocean.ConstructTreeStructures.time_ball_tree('120km')
1.12±0ms 1.11±0.01ms 0.99 mpas_ocean.ConstructTreeStructures.time_ball_tree('480km')
10.6±0.02ms 10.6±0.02ms 1 mpas_ocean.ConstructTreeStructures.time_kd_tree('120km')
650±6ms 652±8ms 1 mpas_ocean.CrossSections.time_const_lat('120km', 1)
330±1ms 331±0.8ms 1 mpas_ocean.CrossSections.time_const_lat('120km', 2)
170±0.5ms 171±2ms 1.01 mpas_ocean.CrossSections.time_const_lat('120km', 4)
487±0.5ms 488±3ms 1 mpas_ocean.CrossSections.time_const_lat('480km', 1)
244±0.8ms 247±2ms 1.01 mpas_ocean.CrossSections.time_const_lat('480km', 2)
126±0.4ms 128±0.5ms 1.01 mpas_ocean.CrossSections.time_const_lat('480km', 4)
22.0±0.02ms 22.3±0.3ms 1.01 mpas_ocean.DualMesh.time_dual_mesh_construction('120km')
2.57±0.01ms 2.64±0.09ms 1.03 mpas_ocean.DualMesh.time_dual_mesh_construction('480km')
838±5ms 845±6ms 1.01 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', False)
49.6±0.7ms 49.6±0.7ms 1 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', True)
73.4±0.9ms 72.9±0.3ms 0.99 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', False)
5.70±0.2ms 5.42±0.1ms 0.95 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', True)
404M 404M 1 mpas_ocean.Gradient.peakmem_gradient('120km')
166±0.7ms 168±4ms 1.01 mpas_ocean.Gradient.time_gradient('120km')
11.4±0.2ms 11.3±0.02ms 0.99 mpas_ocean.Gradient.time_gradient('480km')
190±4μs 189±1μs 1 mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('120km')
87.8±0.4μs 86.6±1μs 0.99 mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('480km')
351M 351M 1 mpas_ocean.Integrate.peakmem_integrate('120km')
331M 330M 1 mpas_ocean.Integrate.peakmem_integrate('480km')
180±1μs 181±1μs 1 mpas_ocean.Integrate.time_integrate('120km')
156±1μs 158±0.6μs 1.01 mpas_ocean.Integrate.time_integrate('480km')
188±2ms 188±2ms 1 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'exclude')
190±3ms 192±3ms 1.01 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'include')
188±1ms 192±2ms 1.02 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'split')
14.0±0.2ms 13.9±0.1ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'exclude')
13.9±0.04ms 13.7±0.04ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'include')
14.0±0.1ms 13.6±0.03ms 0.97 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'split')
281±2μs 284±3μs 1.01 mpas_ocean.PointInPolygon.time_face_search_lonlat('120km')
286±1μs 281±3μs 0.98 mpas_ocean.PointInPolygon.time_face_search_lonlat('480km')
266±1μs 263±1μs 0.99 mpas_ocean.PointInPolygon.time_face_search_xyz('120km')
268±1μs 267±1μs 1 mpas_ocean.PointInPolygon.time_face_search_xyz('480km')
212±1ms 212±0.5ms 1 mpas_ocean.RemapDownsample.time_bilinear_remapping
228±2ms 230±2ms 1.01 mpas_ocean.RemapDownsample.time_inverse_distance_weighted_remapping
4.25±0.05ms 4.26±0.01ms 1 mpas_ocean.RemapDownsample.time_nearest_neighbor_remapping
1.18±0.01s 1.17±0s 1 mpas_ocean.RemapUpsample.time_bilinear_remapping
35.5±0.2ms 36.7±0.6ms 1.03 mpas_ocean.RemapUpsample.time_inverse_distance_weighted_remapping
8.83±0.2ms 9.05±0.09ms 1.03 mpas_ocean.RemapUpsample.time_nearest_neighbor_remapping
29.3±0.4ms 28.6±0.2ms 0.98 mpas_ocean.ZonalAverage.time_zonal_average('120km')
6.13±0.06ms 5.90±0.02ms 0.96 mpas_ocean.ZonalAverage.time_zonal_average('480km')
328M 328M 1 quad_hexagon.QuadHexagon.peakmem_open_dataset
325M 326M 1 quad_hexagon.QuadHexagon.peakmem_open_grid
6.59±0.09ms 6.49±0.09ms 0.99 quad_hexagon.QuadHexagon.time_open_dataset
5.55±0.1ms 5.49±0.06ms 0.99 quad_hexagon.QuadHexagon.time_open_grid

Benchmarks that have got worse:

Change Before [736bc03] After [f3e9ca5] Ratio Benchmark (Parameter)
+ 749±2μs 828±50μs 1.11 mpas_ocean.ConstructTreeStructures.time_kd_tree('480km')

@erogluorhan erogluorhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the fix!

@rajeeja
rajeeja merged commit b0487c4 into main Jul 17, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-benchmark Run ASV benchmark workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When generalizing a structured dataset with UxDataset.from_structured, return information about faces

2 participants