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

Non-Conservative Zonal Mean #1004

Open
wants to merge 77 commits into
base: main
Choose a base branch
from
Open

Non-Conservative Zonal Mean #1004

wants to merge 77 commits into from

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Oct 10, 2024

Closes #93

Overview

@philipc2 philipc2 changed the title DRAFT: Implement Zonal Average using Fast Constant Latitude Cross Sections DRAFT: Zonal Mean using Fast Constant Latitude Cross Sections Oct 10, 2024
@philipc2 philipc2 self-assigned this Oct 10, 2024
uxarray/core/zonal.py Outdated Show resolved Hide resolved
@philipc2
Copy link
Member Author

@erogluorhan @hongyuchen1030 @paullric

For zonal averaging, would the primary difference between Conservative and Non-Conservative be whether we apply a weighted average as opposed to performing a standard average?

@hongyuchen1030
Copy link
Contributor

@erogluorhan @hongyuchen1030 @paullric

For zonal averaging, would the primary difference between Conservative and Non-Conservative be whether we apply a weighted average as opposed to performing a standard average?

As I explained before here #785 (comment)

Both use weighted average

But non-conservative one use the arc length, conservative use the area as weights

@philipc2
Copy link
Member Author

@erogluorhan @hongyuchen1030 @paullric
For zonal averaging, would the primary difference between Conservative and Non-Conservative be whether we apply a weighted average as opposed to performing a standard average?

As I explained before here #785 (comment)

Both use weighted average

But non-conservative one use the arc length, conservative use the area as weights

Sorry for the duplicate! Send this one without seeing your comment before. Appreciate the clarification

@philipc2
Copy link
Member Author

@aaronzedwick

For reference, the pre-commit is failing in all the PR's and CI's too.

@philipc2 philipc2 linked an issue Oct 13, 2024 that may be closed by this pull request
@philipc2 philipc2 changed the title DRAFT: Zonal Mean using Fast Constant Latitude Cross Sections DRAFT: Zonal Mean Nov 8, 2024
@philipc2 philipc2 changed the title DRAFT: Non-Conservative Zonal Mean Non-Conservative Zonal Mean Jan 13, 2025
@philipc2 philipc2 mentioned this pull request Jan 13, 2025
14 tasks
@philipc2 philipc2 marked this pull request as ready for review January 13, 2025 19:43
@philipc2 philipc2 removed the request for review from erogluorhan January 13, 2025 19:45
@philipc2
Copy link
Member Author

There is currently a large bottleneck in perfromance due to the weight computation. See below a profile of the 30km MPAS grid, with latitudes between -45 and 45 degrees using a 2 degree spacing.

image

Despite this, @rajeeja @aaronzedwick @amberchen122 , you may begin to review this PR.

@hongyuchen1030 and I will continue to investigate performance optimizations. If we do not finish them by the time reviews are done, we can continue to iterate after it is merged.

uxarray/core/dataarray.py Outdated Show resolved Hide resolved
@philipc2
Copy link
Member Author

I've improved the performance slightly by removing the newton-raphson solver and by using Polars.

image

test/test_zonal.py Outdated Show resolved Hide resolved
test/test_zonal.py Outdated Show resolved Hide resolved
uxarray/grid/utils.py Outdated Show resolved Hide resolved
uxarray/grid/utils.py Outdated Show resolved Hide resolved
@hongyuchen1030 hongyuchen1030 self-requested a review January 16, 2025 19:02
Copy link
Contributor

@hongyuchen1030 hongyuchen1030 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! Pinning @amberchen122 for a closer look of the implementation

Copy link
Collaborator

@amberchen122 amberchen122 left a comment

Choose a reason for hiding this comment

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

Great job! Thank you for transferring the changes over. Well done!

uxarray/core/dataarray.py Outdated Show resolved Hide resolved
@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Jan 17, 2025
@philipc2 philipc2 removed the run-benchmark Run ASV benchmark workflow label Jan 17, 2025
@philipc2 philipc2 added the run-benchmark Run ASV benchmark workflow label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-benchmark Run ASV benchmark workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add zonal means (non-conservative)
4 participants