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

Use adjust_ps=false in Cxx theta-l_kokkos code #6063

Merged
merged 10 commits into from
Mar 21, 2024
Merged

Use adjust_ps=false in Cxx theta-l_kokkos code #6063

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
057c4f3
Change F90 defaults
tcclevenger Nov 6, 2023
046a080
Remove m_adjust_ps from ForcingFunctor class
tcclevenger Nov 6, 2023
2913989
Use adjusted dp for compute_hydrostatic_p in Cxx
tcclevenger Nov 10, 2023
f61f055
Re-order member initialization
tcclevenger Nov 10, 2023
0e3acbb
Add back m_adjust_ps=true code, condition on dt_remap_factor==0
tcclevenger Dec 21, 2023
a659a3f
change forcing ut to use dt_remap_factor = 1
tcclevenger Jan 8, 2024
9cafd9e
randomize ps, compute dp for forcing ut
tcclevenger Jan 8, 2024
c768be3
Always allocate interface tmp view
tcclevenger Jan 9, 2024
39c2d73
Correct hybrid computation for forcing UT
tcclevenger Feb 19, 2024
eba2ee0
Force adjust_ps=true only if CAM is defined and SCREAM is not defined
tcclevenger Mar 20, 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
14 changes: 7 additions & 7 deletions components/homme/src/share/prim_driver_base.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,16 +1613,16 @@ subroutine applyCAMforcing_tracers(elem,hvcoord,np1,np1_qdp,dt,adjustment)

#ifdef MODEL_THETA_L
if (dt_remap_factor==0) then
adjust_ps=.true. ! stay on reference levels for Eulerian case
adjust_ps=.true. ! stay on reference levels for Eulerian case
else
#ifdef SCREAM
adjust_ps=.false. ! Lagrangian case can support adjusting dp3d or ps
#else
adjust_ps=.true. ! Lagrangian case can support adjusting dp3d or ps
#endif
adjust_ps=.false. ! Lagrangian case can support adjusting dp3d or ps
endif
#else
adjust_ps=.true. ! preqx requires forcing to stay on reference levels
adjust_ps=.true. ! preqx requires forcing to stay on reference levels
#endif

#ifdef CAM
adjust_ps=.true. ! For CAM runs, require forcing to stay on reference levels
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

this last change is the problem for cdahs -- DYAMOND and DPSCREAM tests ( SMS_Ln5.ne30pg2_ne30pg2.F2010-SCREAM-LR-DYAMOND2.chrysalis_intel and SMS_Ln9_P24x1.ne4_ne4.FDPSCREAM-ARM97.chrysalis_intel ) both have -DCAM and -DSCREAM in ./cmake-bld/cmake/atm/CMakeFiles/atm.dir/flags.make defined. We code like only one is active. Previously v0 scream was using adjust_dp3d, but now it is switched to adjust_ps .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to only forcing adjust_ps=.true. is CAM is defined and SCREAM is not defined.


dp=elem%state%dp3d(:,:,:,np1)
Expand Down
13 changes: 6 additions & 7 deletions components/homme/src/theta-l_kokkos/cxx/ForcingFunctor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ class ForcingFunctor
m_hydrostatic = p.theta_hydrostatic_mode;
m_qsize = p.qsize;

// TODO: this may change, depending on the simulation params
m_adjust_ps = true;
m_adjust_ps = (p.dt_remap_factor == 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If p.dt_remap_factor == 0, preserve old behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

by some reason i thought scream is already using adjust_dp3d, but looking at the code it does not. When this goes to scream, it will cause nonbfb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, every test with homme against baselines should fail. I'm going to create a PR when this is merged and look at the diffs.


m_eos.init(m_hydrostatic,m_hvcoord);
m_elem_ops.init(m_hvcoord);
Expand Down Expand Up @@ -370,7 +369,7 @@ class ForcingFunctor
});
if (!m_adjust_ps) {
Kokkos::parallel_for(Kokkos::ThreadVectorRange(kv.team,NUM_LEV),
[&](const int ilev) {
[&](const int ilev) {
dp_adj(ilev) = dp(ilev) + dp(ilev)*(fq(ilev)-q(ilev));
});
}
Expand All @@ -386,7 +385,7 @@ class ForcingFunctor
});
if (!m_adjust_ps) {
Kokkos::parallel_for(Kokkos::ThreadVectorRange(kv.team,NUM_LEV),
[&](const int& ilev) {
[&](const int& ilev) {
dp_adj(ilev) = dp(ilev) + compute_fqdt_pack(ilev,fq,qdp);
});
}
Expand Down Expand Up @@ -472,14 +471,14 @@ class ForcingFunctor
} else {
// Compute hydrostatic p from dp. Store in exner, then add to pnh
auto p_i = Homme::subview(m_pi_i,kv.team_idx,igp,jgp);
m_elem_ops.compute_hydrostatic_p(kv,dp,p_i,exner);
m_elem_ops.compute_hydrostatic_p(kv,dp_adj,p_i,exner);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the change to make F90 BFB. We were not using the correct dp calculation which matches F90.

Kokkos::parallel_for(Kokkos::ThreadVectorRange(kv.team,NUM_LEV),
[&](const int ilev) {
[&](const int ilev) {
pnh(ilev) += exner(ilev);
dp(ilev) = dp_adj(ilev);
});
}

Kokkos::parallel_for(Kokkos::ThreadVectorRange(kv.team,NUM_LEV),
[&](const int ilev) {
using namespace PhysicalConstants;
Expand Down
4 changes: 2 additions & 2 deletions components/homme/src/theta-l_kokkos/cxx/LimiterFunctor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,10 @@ struct LimiterFunctor {
, m_hvcoord(hvcoord)
, m_state(elements.m_state)
, m_geometry(elements.m_geometry)
, m_policy_dp3d_lim (Homme::get_default_team_policy<ExecSpace,TagDp3dLimiter>(m_num_elems))
, m_tu(m_policy_dp3d_lim)
, m_dp3d_thresh(params.dp3d_thresh)
, m_vtheta_thresh(params.vtheta_thresh)
, m_policy_dp3d_lim (Homme::get_default_team_policy<ExecSpace,TagDp3dLimiter>(m_num_elems))
, m_tu(m_policy_dp3d_lim)
Comment on lines -70 to +73
Copy link
Contributor Author

@tcclevenger tcclevenger Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is unrelated, but gives us a warning in EAMxx that I would like to remove. Let me know if I need to revert for the PR.

{
m_np1 = -1;
}
Expand Down