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.

Sign up for GitHub

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

Jump to bottom

Fast Constant Latitude Cross Sections #989

Merged
merged 33 commits into from
Oct 10, 2024
Merged

Fast Constant Latitude Cross Sections #989

merged 33 commits into from
Oct 10, 2024

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Oct 4, 2024 •
edited
Loading

Closes #994 #993 #992

Overview

  • Implements a fast approach to computing constant latitude intersections using Numba
  • Adds a edge_node_z Grid property for caching the z coordinate of the edge nodes
  • API for accessing the edges and face indices for constant latitude cross sections
  • Creates a User Guide section for cross sections.

@review-notebook-app 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 changed the title DRAFT: Fast Cross Sections DRAFT: Fast Constant Latitude Cross Sections Oct 4, 2024
@philipc2 philipc2 added this to the Cross-Sections milestone Oct 4, 2024
This was linked to issues Oct 7, 2024
Fast Constant Latitude Cross Sections #992
Closed
Cross Sections User Guide #993
Closed
Add Grid.edge_node_z #994
Closed
@philipc2 philipc2 changed the title DRAFT: Fast Constant Latitude Cross Sections Fast Constant Latitude Cross Sections Oct 7, 2024
@philipc2 philipc2 marked this pull request as ready for review October 7, 2024 17:22
@philipc2 philipc2 self-assigned this Oct 7, 2024
@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Oct 7, 2024
hongyuchen1030
hongyuchen1030 reviewed Oct 7, 2024
View reviewed changes
test/test_cross_sections.py
uxgrid.constant_latitude_cross_section(lat=10.0)


def test_constant_lat_cross_section_uxds(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some test case when lattitude is around pole area,

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added tests for both poles for a cube-sphere grid.

@philipc2 philipc2 requested a review from rajeeja October 7, 2024 20:30
@philipc2 philipc2 requested a review from rytam2 October 8, 2024 15:46
@philipc2
Copy link
Member Author

philipc2 commented Oct 8, 2024

Here's a figure putting the Github Actions results into perspective

image.

@erogluorhan
Copy link
Member

Could you clarify the addition of docs/user-guide/grid.nc? (I couldn't see its use in the user guide doc, and also even if it will be used, can it be used from the "test/meshfiles" dir?)

@erogluorhan
Copy link
Member

Please clarify if test/meshfiles/ugrid/quad-hexagon/grid.nc is being replaced with a different grid now.

@philipc2
Copy link
Member Author

philipc2 commented Oct 9, 2024

@erogluorhan

The grid the same. I've dropped the edge_node_connectivity that was stored there before when I originally created the grid, the connectivity was invalid.

@philipc2
Copy link
Member Author

philipc2 commented Oct 9, 2024

Could you clarify the addition of docs/user-guide/grid.nc? (I couldn't see its use in the user guide doc, and also even if it will be used, can it be used from the "test/meshfiles" dir?)

That should not be there. Good catch.

erogluorhan
erogluorhan reviewed Oct 9, 2024
View reviewed changes
uxarray/grid/grid.py Outdated Show resolved Hide resolved
erogluorhan
erogluorhan reviewed Oct 9, 2024
View reviewed changes
Copy link
Member

@erogluorhan erogluorhan left a comment

Choose a reason for hiding this comment

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

The overall implementation looks good to me; I've added some comments/questions.

@erogluorhan erogluorhan self-requested a review October 9, 2024 23:53
erogluorhan
erogluorhan approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@erogluorhan erogluorhan 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!

@philipc2 philipc2 merged commit e491f57 into main Oct 10, 2024
19 of 20 checks passed
@philipc2 philipc2 mentioned this pull request Oct 10, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongyuchen1030 hongyuchen1030 hongyuchen1030 left review comments

@erogluorhan erogluorhan erogluorhan approved these changes

@amberchen122 amberchen122 Awaiting requested review from amberchen122

@aaronzedwick aaronzedwick Awaiting requested review from aaronzedwick

@rajeeja rajeeja Awaiting requested review from rajeeja

@rytam2 rytam2 Awaiting requested review from rytam2

Assignees

@philipc2 philipc2

Labels
None yet
Projects
None yet
Milestone
Cross-Sections
Development

Successfully merging this pull request may close these issues.

Add Grid.edge_node_z Cross Sections User Guide Fast Constant Latitude Cross Sections
3 participants
@philipc2 @erogluorhan @hongyuchen1030