Tempo version 3#353
Conversation
AndersJensen-NOAA
requested review from
Qingfu-Liu,
dustinswales,
grantfirl,
joeolson42,
mzhangw and
rhaesung
as code owners
|
@AndersJensen-NOAA Could you please fill out the pull request template so that this PR can be reviewed? |
|
@grantfirl @dustinswales |
@AndersJensen-NOAA I can take a look. Are you sure that they completed? I don't see the /scratch4/BMC/wrfruc/jensen/ufs_tempo_dev_test/tests/logs/RegressionTests_ursa.log file only a backup from yesterday. |
@grantfirl If they did not complete, then I don't know why they didn't. How do I debug that? |
When you ran rt.sh, did you save a log of the output? I'm seeing compilation failures in TEMPO. I see a bunch of |
|
It looks like the TEMPO tests (control_p8_ugwpv1_tempo_aerosol_intel and control_p8_ugwpv1_tempo_intel, regional_wofs_tempo_intel) completed successfully. They failed due to the result change, which is expected. control_wam_debug_gnu failed due to a time-out, which happens occasionally and usually isn't our fault. cpld_debug_sfs_intel, cpld_debug_sfs_intel, cpld_debug_sfs_intelllvm failed due to OOM. It looks like the compilation failures are related to real type errors. It looks like all of the compilation failures are with tests that are varying the default real types. So, I would just doublecheck any recent changes in TEMPO related to real types. |
@grantfirl Thanks! I'll compile the cpld_debug tests and see if I can find the issue. |
|
I need help getting these two regression tests to pass: I modified GFS_rrtmpg_pre for the TEMPO hookup, and now the two mpas tests fail. It appears that those tests are radiation only physics tests specific to MPAS. Since we aren't using MPAS yet in the UFS and since these aren't actually full physics tests, I think those tests should actually be turned off. If not, can one of you fix the TEMPO hookup on the CCPP side? Or better yet, address #361 Thanks. |
We use those tests to keep the MPAS-in-UFS functionality working, so they need to stay. It looks like your RTs were run with the wrong (old) version of TEMPO checked out. I addressed the comment in #353 (comment). Please try to pull down the latest commit of this PR branch with the .gitmodules fix and run Once that's done, please run those 2 failing mpas-based tests again. I'm guessing that it'll fix them. |
Still failed: |
|
Any other ideas, @grantfirl |
|
@AndersJensen-NOAA The FV3 preprocessor flag isn't defined when running with MPAS. Can you remove this #ifdef? Or maybe add the one for MPAS? I'm not sure if it's just MPAS or what. @dustinswales Can you remember when running MPAS in UFS, FV3 isn't defined, but should Anders be putting in a check for |
God find, @grantfirl, thank you. |
|
@grantfirl @dustinswales |
|
Actually, @grantfirl and @dustinswales, the check should go in the radiation code because when using the MPAS dycore, the effective radius values should be passed through from the microphysics. They only need to be calculated when using FV3. |
|
FV3 is not defined when running MPAS in the UFS. As for the radiation coupling... |
| make_DropletNumber_thompson => make_DropletNumber, & | ||
| make_RainNumber_thompson => make_RainNumber | ||
|
|
||
| #ifdef FV3 |
There was a problem hiding this comment.
@AndersJensen-NOAA Can you revert these changes?
There was a problem hiding this comment.
@dustinswales it works, the mpas tests now pass, and it is probably better than unnecessarily exposing internal tempo procedures to mpas when they aren't needed.
There was a problem hiding this comment.
Sure it works, but it's not great to add macros we don't need. There are physics variables you could use to make dycore specific selections in the physics (e.g. here). No need to macros.
But, can you please answer my questions above about changes to TEMPO compared to Thompson?
Previously, with Thompson, all of these routines were made public and referenced in GFS_rrtmg_pre.F90. Why are they now private with TEMPO? (Seems like an MPAS-A specific decision that is having ramifications in the UFS....)
There was a problem hiding this comment.
The Thompson functions , like make_DropletNumber, serve a different purpose. They simply calculate number concentrations if mass mixing ratios exist but number concentrations are too low. They are basically a check on low values. These functions in Thompson have several hard-coded parameter values, and there is no guarantee (given the way the are currently coded) that they will remain physically consistent with Thompson code itself.
My functions are used internally by TEMPO main. They are general procedures for bound checking and updating mass and number concentrations. They are called before diagnostics are output by tempo (reflectivity, effective radius, etc), so they are needed in GFS_rrtmg_pre, but only when effective radii are being calculated, which should not be the case for MPAS.
There was a problem hiding this comment.
The Thompson functions , like make_DropletNumber, serve a different purpose. They simply calculate number concentrations if mass mixing ratios exist but number concentrations are too low. They are basically a check on low values. These functions in Thompson have several hard-coded parameter values, and there is no guarantee (given the way the are currently coded) that they will remain physically consistent with Thompson code itself.
My functions are used internally by TEMPO main. They are general procedures for bound checking and updating mass and number concentrations. They are called before diagnostics are output by tempo (reflectivity, effective radius, etc), so they are needed in GFS_rrtmg_pre, but only when effective radii are being calculated, which should not be the case for MPAS.
My point is that "general" routines in Thompson were public and used by the host models, but this is not true with TEMPO.
Your decision to wall off routines potentially needed by different host models, with coupling different needs, is the issue.
Making these public would avoid the need for macros in two repositories (ccpp and tempo) and have zero consequences within MPAS-A
There was a problem hiding this comment.
WRF and MPAS do things one way, and the UFS/CCPP do things differently. I think we can agree on that. Any so yes, WRF and MPAS specific decisions can have ramifications in the UFS. That is just the way things work. I don't see it as a problem, given that WRF has been around for longer than the UFS and CCPP.
The "general" routines in Thompson are actually in a separate fortran module called module_mp_thompson_make_number_concentrations.F90. They are not in the main Thompson code. So, I could create another module with public routines exposed to the radiation code in a similar way. I don't think I want to add code thought, so I"ll probably just get rid of the FV3 macros.
But, now I need to know how the radiation code will work with MPAS in the UFS? Will there be code added to pass effective radius values to GFS_rrtmg_pre and skip the calculations?
There was a problem hiding this comment.
But, now I need to know how the radiation code will work with MPAS in the UFS? Will there be code added to pass effective radius values to GFS_rrtmg_pre and skip the calculations?
@grantfirl is doing this piece, so you don't need to worry about doing it. You just need TEMPO to work with FV3.
There was a problem hiding this comment.
|
@AndersJensen-NOAA Does this PR need to go back into draft mode? I had approved and had run the UFS RTs for what I thought was the last time before getting this in the UFS merge queue, and then more commits have appeared. Perhaps this is my fault for not commenting that I was doing so. Please let us know when this is ready to go in final form. Also, I noticed that the TEMPO submodule is now pointing to a different branch (main) where the TEMPO submodule commit hash (c237eae42e0dcc8f8b775f114df5c7017e68b84c) doesn't exist. This needs to be updated to the actual commit from the branch that you're pointing to in .gitmodules at this point. In the future, there needs to be a PR in the TEMPO submodule that corresponds to the ccpp-physics PR so this process can be cleaner and we can have an actually static codebase to use for testing. Every time the codebase changes, we need to rerun UFS RTs, which is expensive, and the resources used for RTs are all too finite. |
|
@grantfirl sorry for the miscommunication on this. This is now done, and the ccpp should point to the main branch here: https://github.com/NCAR/TEMPO, which is tagged version 3.1.0. |
8 participants
Description of Changes:
Tempo version 3. See release notes and documentation in the TEMPO: repository https://github.com/NCAR/TEMPO
Tests Conducted:
Tempo Regression tests
Dependencies:
NOAA-EMC/ufsatm#1063
ufs-community/ufs-weather-model#3078
Documentation:
https://ncar.github.io/TEMPO/
Issue (optional):
Contributors (optional):