UXARRAY / uxarray

Xarray extension for unstructured climate and global weather data analysis and visualization.
https://uxarray.readthedocs.io/
Apache License 2.0
148 stars 31 forks source link

Optimize Face Bounds Construction #730

Closed philipc2 closed 4 weeks ago

philipc2 commented 5 months ago

Proposed new feature or change:

Optimize the performance when constructing Grid.face_bounds

erogluorhan commented 5 months ago

Re @philipc2's benchmarking:

The GeoFlow grid (12s execution time) has 6000 nodes and 3840 faces.

That's extremely slow for such a small grid (though relatable as the algorithm iterates over each face, and each edge of that face). Taking into account bigger grids might be used, the execution times might make the things impractical, and that makes me think that we should consider either to optimize before a release, or provide caution admonitions, if possible, in the documentation to let the user clearly know the extreme performance bottleneck.

Thoughts?

Originally posted by @erogluorhan in https://github.com/UXARRAY/uxarray/pull/692#pullrequestreview-1963724660

erogluorhan commented 5 months ago

Some for-loop-based code needs to be replaced with vectorized ones the issue #346. Though, as part of here (if #346 will take too long), should we still find some intermediate (though not ideal) solutions to make our code look better I believe (rather than using a for loop and list appends), e.g. something like the following?:

face_edges_connectivity_cartesian = np.asarray(list(map(node_lonlat_rad_to_xyz, face_edges_connectivity_lonlat)))

_Originally posted by @erogluorhan in https://github.com/UXARRAY/uxarray/pull/692#discussion_r1541423946_

philipc2 commented 5 months ago

We should also investigate the performance of lower-level helper functions that are called throughout.

hongyuchen1030 commented 1 month ago

Hi @erogluorhan @philipc2,

Since the zonal_mean feature is ready for review, can we prioritize the optimization for latlon bounds next? This optimization is the building block for our zonal_mean, a fundamental, core feature that many users are eagerly waiting to use.

Thank you!

erogluorhan commented 1 month ago

Hey @hongyuchen1030, sure!