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

EAMxx: add horizontal average diagnostic field #6788

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mahf708
Copy link
Contributor

@mahf708 mahf708 commented Nov 30, 2024

Adds an online diagnostic field for area-weighted horizontal average, following #6776.


Users are now able to request some_field_horiz_avg in their output files to get an area-weighted horizontal average. The following types of fields are supported:

input dimensions example output dimensions
(COL) area_horiz_avg () (scalar)
(COL, LEV) T_mid_horiz_avg (LEV)
(COL, CMP, LEV) horiz_winds_horiz_avg (CMP, LEV)

@mahf708 mahf708 added the EAMxx PRs focused on capabilities for EAMxx label Nov 30, 2024
@mahf708 mahf708 requested a review from bartgol November 30, 2024 17:48
Copy link

github-actions bot commented Nov 30, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6788/
on branch gh-pages at 2024-12-11 03:01 UTC

@AaronDonahue
Copy link
Contributor

@mahf708 , thanks for re-issuing this PR on the E3SM side. Quick question, there was this discussion between Peter B. and Andrew (E3SM-Project/scream#2738 (comment)) about the algorithm for the horizontal average that takes into account mass. Any thoughts on whether this could/should be incorporated into this PR?

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 2, 2024

@mahf708 , thanks for re-issuing this PR on the E3SM side. Quick question, there was this discussion between Peter B. and Andrew (E3SM-Project/scream#2738 (comment)) about the algorithm for the horizontal average that takes into account mass. Any thoughts on whether this could/should be incorporated into this PR?

This PR is only area-weighted averaging, which is the typical way of doing this as far as I am concerned. I'm happy to hear arguments for including a mass adjustment, but then would the mass adjustment apply to all quantities or specific ones? I'm not sure, but we could easily have another diagnostic such that it is "area-weighted, mass-adjusted" horiz avg

@bogensch and @ambrad, could you please weigh in?

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 2, 2024

I thought dp would be more critical for a vertical aggregator

@bogensch
Copy link
Contributor

bogensch commented Dec 2, 2024

I typically always neglect mass when doing horizontal averages in DP. Unless @ambrad has a strong opinion about including this I'm fine with keeping it area weighted.

@mahf708
Copy link
Contributor Author

mahf708 commented Dec 2, 2024

Yeah. Additionally, I'm not sure if it is realistic or even more accurate?

Also, taking into account vertically resolved mass may become nontrivial when it comes to taking averages with unrelated dimensions (say radiation band or if a field has no vertical dimension). These diag impls are supposed to be generic and applicable to any fields (not just fields similar to dp) ...

@bartgol
Copy link
Contributor

bartgol commented Dec 2, 2024

@AaronDonahue not to digress the PR convo (maybe we can move to slack), but if there's the need to compute "mass" quantities (e.g., dp*some_tracer), we could add a diagnostic mass_X (name up for discussion), which a) ensures X is a 3d quantity for which it makes sense to multiply by dp, and b) computes X(icol,ilev)*dp(icol,ilev). Then, one can request mass_qv_horiz_avg, where IO will first compute mass_qv, and then the corresponding horiz contraction.

Copy link
Contributor

@bartgol bartgol 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. I have a couple of comments, one for testing, and one for the IO mods.

components/eamxx/src/diagnostics/tests/horiz_avg_test.cpp Outdated Show resolved Hide resolved
components/eamxx/src/diagnostics/tests/horiz_avg_test.cpp Outdated Show resolved Hide resolved
components/eamxx/src/share/io/scorpio_output.cpp Outdated Show resolved Hide resolved
@ambrad
Copy link
Member

ambrad commented Dec 2, 2024

I consider mass weighting as probably a bit better in the case of a quantity like a tracer. But I don't think it's terribly wrong to neglect it, except in the case that the diagnostic calculation needs to be fully mass accurate (which probably is never needed).

@mahf708 mahf708 force-pushed the mahf708/eamxx/horiz-avg-diag branch from 3aaf335 to 6ec6f3f Compare December 9, 2024 17:58
@mahf708
Copy link
Contributor Author

mahf708 commented Dec 9, 2024

@bartgol there are still issues to do with composability with the complex logic of field_at_xyz. I can handle those in a later PR, otherwise the scope of this PR will creep again...

Ready for re-review!

Copy link
Contributor

@bartgol bartgol 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. I have one suggestion, but minor.

@mahf708 mahf708 requested a review from bartgol December 11, 2024 02:59
@@ -30,10 +30,12 @@ initialize_impl (const RunType /*run_type*/)
using namespace ShortFieldTagsNames;
const auto& fid = f.get_header().get_identifier();
const auto& layout = fid.get_layout();
EKAT_REQUIRE_MSG (layout.rank()>1 && layout.rank()<=6,
EKAT_REQUIRE_MSG (layout.rank()>=2 && layout.rank()<=6,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry to be annoying here, but I wanted all three checks to be as close as possible. I will edit these entirely in a future PR, when I address the composability

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EAMxx PRs focused on capabilities for EAMxx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants