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

Optimize Face Bounds Construction #730

Closed
philipc2 opened this issue Mar 20, 2024 · 5 comments · Fixed by #877
Closed

Optimize Face Bounds Construction #730

philipc2 opened this issue Mar 20, 2024 · 5 comments · Fixed by #877
Assignees
Labels
new feature New feature or request

Comments

@philipc2
Copy link
Member

Proposed new feature or change:

Optimize the performance when constructing Grid.face_bounds

@erogluorhan
Copy link
Member

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 #692 (review)

@erogluorhan
Copy link
Member

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 #692 (comment)

@philipc2
Copy link
Member Author

philipc2 commented Apr 3, 2024

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

  • _pole_point_inside_polygon
  • extreme_gca_latitude
  • _insert_pt_in_latlonbox
  • _get_cartesian_face_edge_nodes
  • _get_lonlat_rad_face_edge_nodes

@hongyuchen1030
Copy link
Collaborator

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
Copy link
Member

Hey @hongyuchen1030, sure!

@philipc2 philipc2 self-assigned this Aug 6, 2024
@philipc2 philipc2 linked a pull request Aug 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

3 participants