Skip to content

Conversation

@fluidnumerics-joe
Copy link
Collaborator

@fluidnumerics-joe fluidnumerics-joe commented Feb 24, 2025

Closes #1126

Overview

This PR adds the SpatialHash class in the neighbors.py module. This class is used to encapsulate the necessary information and methods for conducting unstructured grid searches using spatial hashes. As part of the initialization of the SpatialHash object, an associated unstructured grid is used to construct a uniformly spaced structured grid, called the "hash grid", and relate cell indices in the hash grid to elements in the unstructured grid. This hash table is a critical element of the spatial hash grid search as it is used to create a short-list of elements to search within.

The SpatialHash.query method returns the element id that a point (or element ids that a list of points) reside within alonside the barycentric coordinates. I've included helper methods to compute the barycentric coordinates of a point in relation to a given unstructured grid face (it works for convex faces n>=3 vertices). The barycentric coordinates are used to determine if a point is inside or outside a face; if all the barycentric coordinates are between [0,1], then a point is inside a face.

Currently, only spherical coordinates (in radians or degrees) are supported for this search.

Expected Usage

import uxarray as ux
grid_path = "../../test/meshfiles/fesom/soufflet-netcdf/grid.nc"
uxgrid = ux.open_grid(grid_path)

uxds = ux.open_dataset(grid_path, data_path)

# Construct the `SpatialHash` object
spatial_hash = uxgrid.get_spatial_hash(
    reconstruct="False",
)

# Query the `SpatialHash` for the face id and the barycentric coordinates.
face_ids, bcoords = spatial_hash.query([0.9, 1.8])

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/

fluidnumerics-joe and others added 4 commits February 18, 2025 16:38
I attempted to stay close to the KD-Tree and BallTree API. However, the
purpose of the SpatialHash class is to help locate the faces that a
list of coordinates lie within. Because of this, the intent of the
`query` function for the SpatialHash is a bit different than that for
the KD and Ball trees.

Additional support routines are provided for assessing whether a point
is inside or outside a polygon. This is done by calculating the
barycentric coordinates of a point in a convex polygon. The method is
defined so that the sum of the barycentric coordinates is exactly one.
Because of this, if any of the barycentric coordinates are negative, we
immediately know that a point is outside the polygon.

All coordinate calculations are done using (lon,lat) in radians for the
SpatialHash class. Cartesian coordinates are not supported in this
commit.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@philipc2 philipc2 added the new feature New feature or request label Feb 24, 2025
@philipc2
Copy link
Member

Hi @fluidnumerics-joe

This looks great so far! May you add a test_spatial_hasing.py module with a few tests and run pre-commit? Let me know if you have any questions.

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 add the spatial-hashing notebook here? (both in the outline and the toc tree at the bottom)

I'm thinking between "Subsetting" and "Cross-Sections?" would be a good spot.

https://github.com/UXARRAY/uxarray/blob/main/docs/userguide.rst?plain=1

@fluidnumerics-joe
Copy link
Collaborator Author

Hi @fluidnumerics-joe

This looks great so far! May you add a test_spatial_hasing.py module with a few tests and run pre-commit? Let me know if you have any questions.

Yes, I'll take care of both of these.

fluidnumerics-joe and others added 8 commits February 26, 2025 17:02
* Ensure that the lon bounds are sorted from low to high. This is needed
to make sure that the index looping for filling the hashtable actually
executes.

* All barycentric coordinates are calculated directly. Rather than
calculating the last coordinate to ensure they sum to one, we now
compute all coordinates. The coordinates are calculated now to align
with the indexing of the node ID's so that np.sum(bcoords*nodes) returns
the barycentric interpolation estimate of point. If the point is within
the element, then the barycentric interpolation estimate matches the
input coordinate (to machine precision). I've added this as a check.
@fluidnumerics-joe
Copy link
Collaborator Author

@philipc2 - I've added tests for the spatial hashing, fixed formatting, and added spatial hashing to the toc in the userguide. Let me know if there's anything else you'd like to see updated

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.

Looks great! A few small comments attached.

I'm going to play around with this today.

Thanks for putting this together so quickly!

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.

Some leftover print statements.

@philipc2
Copy link
Member

philipc2 commented Mar 3, 2025

I'm having some issues with the soufflet-netcdf/grid.nc grid (seems to be crashing the docs too).

It stalls for multiple minutes when trying to construct the tree. This was one of the grids that gave me some trouble in #1013

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.

Looks like one of the Github suggestions broke the pre-commit. Can you run it again?

Also, please restart and clear the outputs of the notebook, they will be rendered via our CI. Other than that all looks good.

@fluidnumerics-joe
Copy link
Collaborator Author

Looks like one of the Github suggestions broke the pre-commit. Can you run it again?

Also, please restart and clear the outputs of the notebook, they will be rendered via our CI. Other than that all looks good.

I re-ran pre-commit run --all-files and cleared python notebook output.

@philipc2
Copy link
Member

philipc2 commented Mar 4, 2025

Looks like one of the Github suggestions broke the pre-commit. Can you run it again?
Also, please restart and clear the outputs of the notebook, they will be rendered via our CI. Other than that all looks good.

I re-ran pre-commit run --all-files and cleared python notebook output.

Oops, look's like its complaining about a merge conflict.

Instead, we're favoring a check of the predicted coordinates using
barycentric interpolation
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.

Thanks for this contribution! Other than the comments I left earlier, it looks good to me.

@aaronzedwick aaronzedwick self-requested a review March 10, 2025 16:35
Copy link
Member

@aaronzedwick aaronzedwick 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 great! I will make use of the barycentric weights in one of my current PR's, so that is really helpful to me. Thanks for your contribution!

@philipc2 philipc2 mentioned this pull request Mar 10, 2025
6 tasks
fluidnumerics-joe and others added 3 commits March 10, 2025 14:02
The denominator in the barycentric weights calculation (Weischell
weights) is clipped to be no smaller than `ERROR_TOLERANCE` from the
uxarray.constants module.

This change is made to fix an issue, reported anectdotally by @Philip2c
in UXARRAY#1169
When the denominator is clipped, rather than the individual triangle
areas, the Weischell weights are incorrect for the case when a query
point is on a face vertex.

I've added tests for query points on edges and vertices
@philipc2
Copy link
Member

@fluidnumerics-joe

Turns out the issue with the 30km MPAS grid was the edge_node_distances were represented in meters, instead of radians, which is inconsistent with our Grid.edge_node_distances definition. The same behavior can be see in the meshfiles\mpas\QU\oQU480.231010.nc grid, but the result is still correct. This was causing the hash cell size to be much larger than exepcted.

It also appears we have a typo in our Grid.edge_node_distances definition. The unit should be radians instead of degrees here

uxarray/uxarray/grid/grid.py

Lines 1386 to 1394 in d9f091f

@property
def edge_node_distances(self):
"""Distances between the two nodes that surround each edge in degrees.
Dimensions ``(n_edge, )``
"""
if "edge_node_distances" not in self._ds:
_populate_edge_node_distances(self)
return self._ds["edge_node_distances"]

I'll raise an issue and get this fixed. Glad to see that the issue wasn't anything with your implementation.

@philipc2 philipc2 merged commit 2154ea8 into UXARRAY:main Mar 10, 2025
20 checks passed
@fluidnumerics-joe
Copy link
Collaborator Author

🥳 - thanks @philipc2 and @aaronzedwick for your feedback. Without it, I would have not seen some of the cases that we ended up building tests for. This is great to see pushed across the finish line.

Feel free to reach out on any future issues related to the spatial hashing. I'm happy to chip in.

@rajeeja
Copy link
Contributor

rajeeja commented Mar 11, 2025

@fluidnumerics-joe

Thanks for this work and hoping to see few more features in future :)

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

None yet

Development

Successfully merging this pull request may close these issues.

Suggested Feature : Spatial Hashing and Point Cloud Interpolation

4 participants