Tempo Implementation#250
Conversation
AndersJensen-NOAA
requested review from
Qingfu-Liu,
dustinswales,
grantfirl,
mzhangw and
rhaesung
as code owners
|
@AndersJensen-NOAA Is this the final PR? It makes it very difficult to review this when new PRs replace old ones. For example, in order to see what comments were addressed from #245, I have to go back and forth between that one and this one and check for changes manually. If you had just made your changes in #245, it would have been much easier to continue reviewing this because GitHub highlights changes for you. |
@grantfirl Yes, this is the final PR. Apologies for the continued replacement PRs. The issue mentioned in this discussion |
| [submodule "physics/MP/TEMPO/TEMPO"] | ||
| path = physics/MP/TEMPO/TEMPO | ||
| url = https://github.com/NCAR/TEMPO | ||
| branch = main No newline at end of file |
There was a problem hiding this comment.
I noticed that the TEMPO submodule pointer isn't pointing to the latest commit of the main branch. Is this intentional?
There was a problem hiding this comment.
@grantfirl , Yes TEMPO has moved forward with MPAS development since the PR to ccpp.
| qs_mp (i,k) = tracer1(i,k,ntsw)/(1.-qvs) | ||
| if(nint(slmsk(i)) == 1) then | ||
| nc_mp (i,k) = Nt_c_l*orho(i,k) | ||
| if (imp_physics == imp_physics_thompson) then |
There was a problem hiding this comment.
Same comment as https://github.com/ufs-community/ccpp-physics/pull/245/files#r1907698791. If performance isn't a concern right now, then you can disregard.
| endif | ||
|
|
||
| if (imp_physics == imp_physics_thompson .and. (ntlnc>0 .or. ntinc>0)) then | ||
| if ((imp_physics == imp_physics_thompson .or. imp_physics == imp_physics_tempo) .and. & |
| if (is_initialized) return | ||
|
|
||
| ! Set local TEMPO MP module constants from host model | ||
| ! PI = con_pi |
There was a problem hiding this comment.
Why are these commented out? With the code as-is, this scheme is using the redefined "default" values of these constants in the TEMPO repo, module_mp_tempo_params.F90, right? This allows this scheme to introduce inconsistencies with the rest of the physics and the host with respect to physical constants.
There was a problem hiding this comment.
@grantfirl, The quickest way to implement the PR was to use the constants from TEMPO. These parameters can (and probably) should be brought in. When should that be done, before or after this merges?
|
@AndersJensen-NOAA Everything looks good from a CCPP point of view with the exception of not actually using the constants coming from the host in the init phase. The 'if' statements within loops over the domain are slow, but don't have to be fixed if speed doesn't matter much right now. I went into the TEMPO repo and reviewed for CCPP problems in module_mp_tempo_main.F90 as well. I'll approve once the physical constants problem is worked out. For future PRs, where changes within the TEMPO submodule are made, there will be PRs to update that main branch, right? Plus, there will be periodic PRs into the ccpp-physics branch to update the TEMPO submodule hash as development continues, correct? Otherwise, if a bunch of development goes into the main branch of TEMPO without PRs, that will lead to hard-to-review ccpp-physics PRs in the future. |
|
@AndersJensen-NOAA Also, please be sure to fill out the PR description here (should just be a copy/paste from previous PRs). |
@grantfirl @dustinswales Do I need to update the constants or can one of you do that update? In the future, updates to TEMPO will go to the main repository and there will be period PRs to ccpp. The frequency of PRs to ccpp is yet-to-be-determined, but we should figure out a cadence that works well for everyone. Of course, big changes should be merged faster, while smaller changes can likely accumulate unless they are major bug fixes. |
@AndersJensen-NOAA Could you please add my GitHub user as a collaborator that can push to your forks of ccpp-physics, fv3atm, and ufs-weather-model? If there is a shutdown, I can push updates to your PRs and we can continue testing for the merge queue as long as HPC platforms are still up. Also, I can make the physical constants change that I suggested. |
|
@grantfirl The green check turned into a red x. Is there anything that I need to do? What is the status on this? |
|
@AndersJensen-NOAA There has been a change to GitHub's billing policy for CI testing in forked repositories, NCAR#1138 (comment), so we shut off CI in forked repos. @scrasmussen It it expected that the CI test would be stalled like it is? |
@AndersJensen-NOAA You can safely ignore this. There is a good chance that this will get merged in the next couple of weeks. There's nothing to do except occasionally updating this to the latest develop code, which I've been doing periodically. |
|
@AndersJensen-NOAA @dustinswales This commit should fix the failure once we push from NCAR:main to ufs/dev: NCAR@e45f198 @scrasussen fixed this in the NCAR:main repo. |
|
Combined with #195 |
4 participants
This PR replaces #245
Development of the Thompson-Eidhammer microphysics is transitioning to a submodule, and this submodule is: https://github.com/NCAR/TEMPO
TEMPO stand for Thompson-Eidhammer Microphysics Parameterization for Operations.
The use of a submodule will enable centralized community development (and testing) of the microphysics scheme for various applications and dynamical cores. This submodule will eventually replace the Thompson parameterization currently in the CCPP after approval from all parties involved and significant testing.