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

Latitude & Longitude Interval Cross Section, Updated Bounding Box Subset #1115

Merged
merged 12 commits into from
Jan 17, 2025

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Dec 19, 2024

Closes #1104

Overview

  • Introduces bounded latitude & longitude cross sections, which returns a new grid with elements strictly between two lines of constant latitude or longitude
  • Updates bounding box subset to use the spherical bounds instead of KDTree queries

@philipc2 philipc2 self-assigned this Dec 19, 2024
@philipc2 philipc2 linked an issue Dec 19, 2024 that may be closed by this pull request
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@philipc2 philipc2 requested a review from aaronzedwick January 10, 2025 18:31
@philipc2 philipc2 marked this pull request as ready for review January 10, 2025 18:31
@aaronzedwick
Copy link
Member

Glad to see this, definitely nice to have this updated. Quick clarification, does this update the bounding box and cross-section to be strictly between those bounds? So if I do a bounding box with a face center as the element, and a face center is inside the bounds but the whole face is not, will it include that face or not?

@aaronzedwick
Copy link
Member

Not using KDTree queries will definitely speed things up though, so that is really good.

@philipc2
Copy link
Member Author

Glad to see this, definitely nice to have this updated. Quick clarification, does this update the bounding box and cross-section to be strictly between those bounds? So if I do a bounding box with a face center as the element, and a face center is inside the bounds but the whole face is not, will it include that face or not?

The subset.bounding_box() methods now use the Grid.bounds to accurately determine the bounding box, instead of using an approximate query.

So if I do a bounding box with a face center as the element, and a face center is inside the bounds but the whole face is not, will it include that face or not?

There is no longer an element needed, since we are not using the coordinates for the query, instead we use the actual geometry.

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.

good addition, will review more today.

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

looks good, minor comment for tests



def test_constant_longitude_interval_grid():
uxgrid = ux.open_grid(quad_hex_grid_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

some comment on what is in quad hex grid and why the lon/lat ranges in test here return the faces as shown in asserts would help the readability of the code.

@philipc2 philipc2 merged commit 78dca67 into main Jan 17, 2025
16 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.

Update .subset.bounding_box() to use spherical face bounds
3 participants