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

Add weighted_average_lat #39

Merged
merged 3 commits into from
Jul 26, 2024
Merged

Add weighted_average_lat #39

merged 3 commits into from
Jul 26, 2024

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Jul 24, 2024

And add option to add weights to average_lat,

I didn't make it default to maintain backwards compatibility, but we can make it default in the future.

Closes #38

@Sbozzolo Sbozzolo requested a review from szy21 July 24, 2024 09:36
@Sbozzolo Sbozzolo force-pushed the gb/reorganize branch 2 times, most recently from e5902ec to fb86e46 Compare July 24, 2024 09:46
Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.62%. Comparing base (e1d62f2) to head (ce1c74a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   94.39%   94.62%   +0.22%     
==========================================
  Files           7        7              
  Lines         357      372      +15     
==========================================
+ Hits          337      352      +15     
  Misses         20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few comments.

src/Var.jl Outdated
Comment on lines 217 to 219
maximum(lats) >= 0.5π ||
@warn "Detected latitudes are small. If units are radians, results will be wrong"

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
maximum(lats) >= 0.5π ||
@warn "Detected latitudes are small. If units are radians, results will be wrong"
maximum(abs(lats)) >= 0.5π ||
@warn "Detected latitudes are small. If units are radians, results will be wrong"

(it is common to take e.g. southern hemisphere mean, and this way the warning wouldn't show up for that case)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!


weights[index_tuple...] .= weights_1d
end
var.data .*= weights
Copy link
Member

Choose a reason for hiding this comment

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

If _reduce_over takes the average, the weights here should be divided by the average weights?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I need to normalize the weights!

Copy link
Member

Choose a reason for hiding this comment

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

If you do this the average of var should be sum(var.data) instead of mean(var.data), because the sum of the weights is 1 now? Alternatively we can do weights ./= mean(weights_1d) I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thank you! I changed to mean

@szy21
Copy link
Member

szy21 commented Jul 24, 2024

@cmschmitt519 You can use this once it's merged.

@Sbozzolo
Copy link
Member Author

@szy21 can you have another look?

Copy link
Member

@szy21 szy21 left a comment

Choose a reason for hiding this comment

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

Thanks!

@Sbozzolo Sbozzolo merged commit c8314a7 into main Jul 26, 2024
5 checks passed
@Sbozzolo
Copy link
Member Author

@cmschmitt519 If you need this, it is available in the most recent version (0.5.6)

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.

Use weighted average in average_lat
2 participants