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.

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

Normalization of Parsed Cartesian Coordinates #878

Merged
merged 17 commits into from
Sep 18, 2024
Merged

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Aug 6, 2024

Closes #872

Overview

  • Adds a helper uxarray.grid.validation._check_normalization() to check if the Cartesian coordinates stored under a Grid are normalized.
  • Adds Grid.normalize_cartesian_coordinates() to normalize coordinates if they were initially stored as non-normalized.

Expected Usage

import uxarray as ux

grid_path = "/path/to/grid.nc"
data_path = "/path/to/data.nc"

uxds = ux.open_dataset(grid_path, data_path)

# normalize coordinates
uxds.uxgrid.normalize_cartesian_coordinates()

uxarray/grid/validation.py Outdated Show resolved Hide resolved
@philipc2
Copy link
Member Author

philipc2 commented Sep 6, 2024

@hongyuchen1030

Do you recall which MPAS dataset had the non-normalized Cartesian coordinates? The two we have in testing both are already normalized.

EDIT: Might have checked something wrong, looking into it now

@philipc2 philipc2 self-assigned this Sep 6, 2024
@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Sep 6, 2024
@hongyuchen1030
Copy link
Contributor

@hongyuchen1030

We should expect the coordinates to be in the [-1, 1] range as well correct?

Yes, correct.

And unfortunately, I don't remember which dataset I ran into, but I will like you know once I saw it.

Copy link

github-actions bot commented Sep 6, 2024

ASV Benchmarking

Benchmark Comparison Results

Benchmarks that have improved:

Change Before [92f38db] After [2b92c7b] Ratio Benchmark (Parameter)
failed 659±7μs n/a mpas_ocean.CheckNorm.time_check_norm('120km')
failed 430±7μs n/a mpas_ocean.CheckNorm.time_check_norm('480km')
- 408M 354M 0.87 mpas_ocean.Integrate.peakmem_integrate('480km')

Benchmarks that have stayed the same:

Change Before [92f38db] After [2b92c7b] Ratio Benchmark (Parameter)
failed failed n/a face_bounds.Bounds.time_bounds
383M 383M 1.00 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
385M 384M 1.00 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
384M 387M 1.01 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
382M 385M 1.01 face_bounds.FaceBounds.peakmem_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
1.57±0.01s 1.57±0.01s 1.00 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/mpas/QU/oQU480.231010.nc'))
226±3ms 222±2ms 0.98 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/scrip/outCSne8/outCSne8.nc'))
2.02±0.01s 2.02±0.01s 1.00 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/geoflow-small/grid.nc'))
7.65±0.1ms 7.74±0.2ms 1.01 face_bounds.FaceBounds.time_face_bounds(PosixPath('/home/runner/work/uxarray/uxarray/test/meshfiles/ugrid/quad-hexagon/grid.nc'))
1.77±0.01s 1.76±0.02s 0.99 import.Imports.timeraw_import_uxarray
652±5ms 642±6ms 0.99 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('120km')
42.0±0.7ms 41.1±0.7ms 0.98 mpas_ocean.ConnectivityConstruction.time_face_face_connectivity('480km')
493±4μs 506±10μs 1.03 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('480km')
1.33±0.01μs 1.31±0.01μs 0.99 mpas_ocean.ConstructTreeStructures.time_ball_tree('120km')
304±2ns 313±0.9ns 1.03 mpas_ocean.ConstructTreeStructures.time_ball_tree('480km')
867±6ns 860±5ns 0.99 mpas_ocean.ConstructTreeStructures.time_kd_tree('120km')
291±5ns 300±4ns 1.03 mpas_ocean.ConstructTreeStructures.time_kd_tree('480km')
399M 399M 1.00 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('120km', False)
389M 389M 1.00 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('120km', True)
362M 362M 1.00 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('480km', False)
361M 361M 1.00 mpas_ocean.GeoDataFrame.peakmem_to_geodataframe('480km', True)
1.04±0.01s 1.05±0.01s 1.01 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', False)
57.7±0.4ms 58.3±0.5ms 1.01 mpas_ocean.GeoDataFrame.time_to_geodataframe('120km', True)
78.3±0.3ms 80.1±0.6ms 1.02 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', False)
5.57±0.2ms 5.88±0.2ms 1.06 mpas_ocean.GeoDataFrame.time_to_geodataframe('480km', True)
264M 263M 1.00 mpas_ocean.Gradient.peakmem_gradient('120km')
241M 241M 1.00 mpas_ocean.Gradient.peakmem_gradient('480km')
2.69±0.01ms 2.70±0.02ms 1.00 mpas_ocean.Gradient.time_gradient('120km')
289±4μs 290±1μs 1.01 mpas_ocean.Gradient.time_gradient('480km')
243±2μs 237±3μs 0.98 mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('120km')
122±0.7μs 121±0.8μs 0.99 mpas_ocean.HoleEdgeIndices.time_construct_hole_edge_indices('480km')
370M 371M 1.00 mpas_ocean.Integrate.peakmem_integrate('120km')
177±1ms 173±0.7ms 0.98 mpas_ocean.Integrate.time_integrate('120km')
12.0±0.08ms 11.9±0.05ms 0.99 mpas_ocean.Integrate.time_integrate('480km')
349±3ms 350±3ms 1.00 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'exclude')
347±4ms 348±2ms 1.00 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'include')
349±4ms 350±0.5ms 1.00 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('120km', 'split')
22.9±0.4ms 22.3±0.2ms 0.97 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'exclude')
22.8±0.7ms 22.6±0.5ms 0.99 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'include')
22.7±0.4ms 22.6±0.1ms 1.00 mpas_ocean.MatplotlibConversion.time_dataarray_to_polycollection('480km', 'split')
54.9±0.2ms 53.7±0.3ms 0.98 mpas_ocean.RemapDownsample.time_inverse_distance_weighted_remapping
44.4±0.2ms 43.5±0.05ms 0.98 mpas_ocean.RemapDownsample.time_nearest_neighbor_remapping
361±1ms 356±1ms 0.99 mpas_ocean.RemapUpsample.time_inverse_distance_weighted_remapping
263±0.3ms 260±0.5ms 0.99 mpas_ocean.RemapUpsample.time_nearest_neighbor_remapping
237M 237M 1.00 quad_hexagon.QuadHexagon.peakmem_open_dataset
236M 236M 1.00 quad_hexagon.QuadHexagon.peakmem_open_grid
6.66±0.05ms 6.49±0.07ms 0.97 quad_hexagon.QuadHexagon.time_open_dataset
5.78±0.07ms 5.63±0.04ms 0.97 quad_hexagon.QuadHexagon.time_open_grid

Benchmarks that have got worse:

Change Before [92f38db] After [2b92c7b] Ratio Benchmark (Parameter)
+ 1.86±0.2ms 2.17±0.03ms 1.17 mpas_ocean.ConnectivityConstruction.time_n_nodes_per_face('120km')

uxarray/grid/validation.py Outdated Show resolved Hide resolved
@philipc2 philipc2 marked this pull request as ready for review September 6, 2024 19:43
@philipc2 philipc2 changed the title DRAFT: Normalization of Parsed Cartesian Coordinates Normalization of Parsed Cartesian Coordinates Sep 6, 2024
@philipc2 philipc2 requested review from rajeeja September 9, 2024 16:08
test/test_grid.py Outdated Show resolved Hide resolved
@philipc2
Copy link
Member Author

@hongyuchen1030 @amberchen122

This should be ready for review.

@philipc2 philipc2 removed the run-benchmark Run ASV benchmark workflow label Sep 17, 2024
@philipc2 philipc2 mentioned this pull request Sep 18, 2024
14 tasks
Copy link
Member

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

This looks good to me with the application of global error tolerance to isclose checks.

@rajeeja rajeeja merged commit 0f7282c into main Sep 18, 2024
21 checks passed
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.

Inconsistent Normalization of Cartesian Coordinates in node_x, node_y, and node_z
4 participants