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

Regional Delaunay #1052

Merged
merged 19 commits into from
Nov 15, 2024
Merged

Regional Delaunay #1052

merged 19 commits into from
Nov 15, 2024

Conversation

aaronzedwick
Copy link
Member

@aaronzedwick aaronzedwick commented Oct 30, 2024

Closes #1049 #1050 #1051

Overview

Adds Grid.to_planar which takes a grid and projects it to a plane, using _point_to_plane which converts a point on the sphere to the plane. Also adds _point_to_sphere which converts a point on a plane back to the sphere.

Expected Usage

import uxarray as ux

grid_points = [[10, -10, 10, -10], [-10, -10, 10, 10]]

grid_r = ux.Grid.from_points(grid_points, method="regional_delaunay")

PR Checklist

General

  • An issue is linked created and linked
  • Add appropriate labels
  • Filled out Overview and Expected Usage (if applicable) sections

Testing

  • Adequate tests are created if there is new functionality
  • Tests cover all possible logical paths in your function
  • Tests are not too basic (such as simply calling a function and nothing else)

Documentation

  • Docstrings have been added to all new functions
  • Docstrings have updated with any function changes
  • Internal functions have a preceding underscore (_) and have been added to docs/internal_api/index.rst
  • User functions have been added to docs/user_api/index.rst

Examples

  • Any new notebook examples added to docs/examples/ folder
  • Clear the output of all cells before committing
  • New notebook files added to docs/examples.rst toctree
  • New notebook files added to new entry in docs/gallery.yml with appropriate thumbnail photo in docs/_static/thumbnails/

@aaronzedwick aaronzedwick self-assigned this Oct 30, 2024
@aaronzedwick
Copy link
Member Author

@philipc2 got any suggestions on how I might store this information? I am not completely certain about a solid way to do so.

@philipc2
Copy link
Member

@philipc2 got any suggestions on how I might store this information? I am not completely certain about a solid way to do so.

Do these variables need to be cached, or can they be used once and then scrapped? Considering the computation of these variables will be pretty quick thanks to Numba, I lean towards not storing them directly as part of a Grid.

@aaronzedwick
Copy link
Member Author

@philipc2 got any suggestions on how I might store this information? I am not completely certain about a solid way to do so.

Do these variables need to be cached, or can they be used once and then scrapped? Considering the computation of these variables will be pretty quick thanks to Numba, I lean towards not storing them directly as part of a Grid.

Sorry, I think I miscommunication. I more meant, how should I return this information? Is using a dataset the best way, and constructing them all at once? Or should I have one function for each (node, face, edge) and return a np array?

@philipc2
Copy link
Member

@philipc2 got any suggestions on how I might store this information? I am not completely certain about a solid way to do so.

Do these variables need to be cached, or can they be used once and then scrapped? Considering the computation of these variables will be pretty quick thanks to Numba, I lean towards not storing them directly as part of a Grid.

Sorry, I think I miscommunication. I more meant, how should I return this information? Is using a dataset the best way, and constructing them all at once? Or should I have one function for each (node, face, edge) and return a np array?

I see! I'd lean towards not having a Grid.to_planar() method and instead having the _point_to_plane() and _point_to_sphere() called as needed, returning the variables instead of storing them as a dataset.

@aaronzedwick aaronzedwick changed the title DRAFT: Add Grid.to_planar DRAFT: Add Regional Delaunay Nov 1, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@aaronzedwick aaronzedwick marked this pull request as ready for review November 8, 2024 15:32
@aaronzedwick aaronzedwick changed the title DRAFT: Add Regional Delaunay Regional Delaunay Nov 8, 2024
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.

Can you test the behavior if the points are along the antemeridian? If you want to use data from the usage example, you could try the following.

uxgrid_regional.subset.bounding_circle((-180, 0), 10)

uxarray/grid/geometry.py Outdated Show resolved Hide resolved
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.

Can you also expand upon the markdown in the notebook? I think the most important addition would be the "How It Works" section that describes the underlying algorithm.

image

@aaronzedwick
Copy link
Member Author

@philipc2 This projection technique (sterographic) can't handle south pole points, or rather any points along the z=1 axis. I was thinking maybe using a gnomonic projection would work better and be more accurate (as this one seems to not work very well for point in polygon either, the projection is distorted and gives incorrect results when the point is close to the edge). The gnomonic projection might have the same issue at the poles though, not certain. Thoughts? Do you think gnomonic projection would give more accurate results?

Copy link
Contributor

@rajeeja rajeeja left a comment

Choose a reason for hiding this comment

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

can you do something like norm = np.linalg.norm(points, axis=1) to calculate all norm at once?

also, is might faster to use set operation for filtering.

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.

Notebook changes are looking great!

One small comment below.

uxarray/grid/grid.py Show resolved Hide resolved
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.

Great work @aaronzedwick

@rajeeja rajeeja merged commit fd757b0 into main Nov 15, 2024
20 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.

Implement Regional Delaunay Construct connectivity from regional points using Delaunay Triangulation
3 participants