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

SHOC related diagnostic output #2975

Merged
merged 2 commits into from
Sep 9, 2024
Merged

SHOC related diagnostic output #2975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
004c187
add several SHOC variables to be output for diagnostic purposes
Aug 28, 2024
ead3cb7
add runtime guard around added diagnostics
mahf708 Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/eamxx/cime_config/namelist_defaults_scream.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ be lost if SCREAM_HACK_XML is not enabled.
<c_diag_3rd_mom type="real" doc="Third moment vertical velocity damping factor">7.0</c_diag_3rd_mom>
<Ckh type="real" doc="Eddy diffusivity coefficient for heat">0.1</Ckh>
<Ckm type="real" doc="Eddy diffusivity coefficient for momentum">0.1</Ckm>
<extra_shoc_diags type="logical" doc="Extra SHOC diagnostics">false</extra_shoc_diags>
</shoc>

<!-- MAM4xx-ACI -->
Expand Down
54 changes: 54 additions & 0 deletions components/eamxx/src/physics/shoc/eamxx_shoc_process_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,25 @@ void SHOCMacrophysics::set_grids(const std::shared_ptr<const GridsManager> grids
add_field<Computed>("ustar", scalar2d, m/s, grid_name, ps);
add_field<Computed>("obklen", scalar2d, m, grid_name, ps);

// Extra SHOC output diagnostics
if (m_params.get<bool>("extra_shoc_diags", false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

While this option makes sense to avoid polluting the FM with stuff we don't need, it has the drawback that users must atmchange this whenever they add one of these fields to their output yaml files.

I suppose this is the best we can do for now. Hopefully this will become obsolete once we rework a bit the IO layer for optional outputs.


// Diagnostic output - mid point grid
add_field<Computed>("brunt", scalar3d_mid, pow(s,-1), grid_name, ps);
add_field<Computed>("shoc_mix", scalar3d_mid, m, grid_name, ps);
add_field<Computed>("isotropy", scalar3d_mid, s, grid_name, ps);

// Diagnostic output - interface grid
add_field<Computed>("wthl_sec", scalar3d_int, K*(m/s), grid_name, ps);
add_field<Computed>("thl_sec", scalar3d_int, pow(K,2), grid_name, ps);
add_field<Computed>("wqw_sec", scalar3d_int, (kg/kg)*(m/s), grid_name, ps);
add_field<Computed>("qw_sec", scalar3d_int, pow(kg/kg,2), grid_name, ps);
add_field<Computed>("uw_sec", scalar3d_int, pow(m/s,2), grid_name, ps);
add_field<Computed>("vw_sec", scalar3d_int, pow(m/s,2), grid_name, ps);
add_field<Computed>("w3", scalar3d_int, pow(m/s,3), grid_name, ps);

} // Extra SHOC output diagnostics

// Tracer group
add_group<Updated>("tracers", grid_name, ps, Bundling::Required);

Expand Down Expand Up @@ -495,6 +514,41 @@ void SHOCMacrophysics::run_impl (const double dt)
default_policy,
shoc_postprocess);
Kokkos::fence();

// Extra SHOC output diagnostics
if (m_params.get<bool>("extra_shoc_diags", false)) {

const auto& shoc_mix = get_field_out("shoc_mix").get_view<Spack**>();
Kokkos::deep_copy(shoc_mix,history_output.shoc_mix);
Copy link
Contributor

@bartgol bartgol Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is indeed costly, for no reason. In principle, we would like history_output.shoc_mix to simply use the view stored inside the Field "shoc_mix". We are basically adding 10 3d fields for no real reason...

OTOH, this may add more complexity, since we need to be careful during init, depending on the value of extra_shoc_diags:

  • false: request enough buffer mem to accommodate these vars.
  • true: do NOT request buffer mem for these fields, and grab them from the computed field during initialize_impl

I still think keeping memory under control in this case is doable with not too much effort...

Copy link
Contributor

Choose a reason for hiding this comment

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

The buffer needs a bit of a reworked to allow for it to be dynamically sized based on a param and not be unreadable. @bogensch If you do not want to do that I can take a look. But at the very least we can get rid of the deep copies like Luca said, in initialize_impl:

if(m_params.get<bool>("extra_shoc_diags")) {
  history_output.shoc_mix = get_field_out("shoc_mix").get_view<Spack**>();
} else {
  history_output.shoc_mix = m_buffer.shoc_mix;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Not too much logic to add. When SHOCMacrophysics::requested_buffer_size_in_bytes and SHOCMacrophysics::init_buffers are called, we can ping m_params, to see if we need the extra mem for those fields or not.

Copy link
Contributor

@tcclevenger tcclevenger Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You're right, it's not too much logic. But we use a constexpr for number of fields in the buffer, and then we initialize the buffer views in a way that probably doesn't lend itself to using if-else nicely. So I think some structural tweaks will need to happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tcclevenger what's your plan regarding these unnecessary copies? Are you planning to fix them here or in another PR? If the latter, you can go ahead and merge, since the cosp failures are orthogonal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll merge and include this fix in new diag output work.


const auto& brunt = get_field_out("brunt").get_view<Spack**>();
Kokkos::deep_copy(brunt,history_output.brunt);

const auto& w3 = get_field_out("w3").get_view<Spack**>();
Kokkos::deep_copy(w3,history_output.w3);

const auto& isotropy = get_field_out("isotropy").get_view<Spack**>();
Kokkos::deep_copy(isotropy,history_output.isotropy);

const auto& wthl_sec = get_field_out("wthl_sec").get_view<Spack**>();
Kokkos::deep_copy(wthl_sec,history_output.wthl_sec);

const auto& wqw_sec = get_field_out("wqw_sec").get_view<Spack**>();
Kokkos::deep_copy(wqw_sec,history_output.wqw_sec);

const auto& uw_sec = get_field_out("uw_sec").get_view<Spack**>();
Kokkos::deep_copy(uw_sec,history_output.uw_sec);

const auto& vw_sec = get_field_out("vw_sec").get_view<Spack**>();
Kokkos::deep_copy(vw_sec,history_output.vw_sec);

const auto& qw_sec = get_field_out("qw_sec").get_view<Spack**>();
Kokkos::deep_copy(qw_sec,history_output.qw_sec);

const auto& thl_sec = get_field_out("thl_sec").get_view<Spack**>();
Kokkos::deep_copy(thl_sec,history_output.thl_sec);

mahf708 marked this conversation as resolved.
Show resolved Hide resolved
} // Extra SHOC output diagnostics
}
// =========================================================================================
void SHOCMacrophysics::finalize_impl()
Expand Down