RUC LSM update #1940
RUC LSM update #1940
Conversation
|
@SamuelTrahanNOAA Sam, I would like to add you as a reviewer. |
|
Your hera test logs say the tests are still running. You'll need to add the hera test log again and push it once the tests have completed. |
|
@SamuelTrahanNOAA Yes, I will update the log when the full regression test passes. |
…f the UFS/develop and some small changes to address reviewer's comments.
|
@tanyasmirnova I see that the RT log that you pushed shows success when testing against your own baseline, but shouldn't there be RT failures testing against the existing ufs-community baselines? |
|
@tanyasmirnova @jkbk2004 @joeolson42 There is a desire to combine this PR with #1945 to save time in the queue. @joeolson42 is going to run RTs on #1945 once @AndersJensen-NOAA pushes some small changes to verify that the correct RTs are failing. Once we know that the RTs for this PR are failing as expected (if any), we should be OK to combine them. @tanyasmirnova I can re-run RTs for this one against the ufs-community baselines and report the list of failed RTs (if any) here. |
|
@grantfirl Grant, before making my own baseline I compared against the ufs-community baseline, and only suites containing RUC LSM failed. After that I created my own baseline. I would appreciate if you could rerun the RT against ufs-community baselines. |
BrianCurtis-NOAA
added
Baseline Updates
|
The following tests failed in check_result. @tanyasmirnova This seems like more tests than I expected, including some that don't use RUC LSM. Does this make sense to you? 003 cpld_control_gfsv17_iau_intel failed in check_result |
|
@tanyasmirnova @SamuelTrahanNOAA Do either of you know if the list of failed RTs above are all expected failures? I want to merge this with Joe's PR, but I can't until I understand whether these failures are expected given the code changes. |
|
@grantfirl @SamuelTrahanNOAA Grant and Sam, I expected all RAP and HRRR tests to fail, but I am not sure why, for example, cpld_control_gfsv17_iau_intel test did not pass. I am trying to run a regression test in my directory on hera to see if I can confirm the same failures of non_RUC SDFs. Sam, do you have time to look into this? Thank you! |
|
@grantfirl @SamuelTrahanNOAA To me the only suspect is GFS_diagnostics.F90. I have also merged to my PR and upstream/develp on October 18, maybe this could change the results? |
You should check the diagnostic fields. I'm wondering if your changes to GFS_diagnostics.F90 has had unintended effects on other configurations' output. I see nothing in the physics or GFS_typedefs that could affect non-ruc non-smoke configurations. |
Yes, this is why I was confused as well. FYI, I ran regression tests both using this branch as-is and after updating to the latest develop branch, and I got the exact same failures. I also ran regression tests only omitting the changes to GFS_diagnostics.F90 in the fv3atm PR and also got the same failures. For example, for the cpld_control_gfsv17_iau_intel test, only the GFSPRS.GrbF12 file is not identical. |
|
I will double-check that the baseline is OK by running one of the tests that we don't expect to fail (cpld_control_gfsv17_iau_intel) with the top of the develop branch. |
|
@grantfirl @SamuelTrahanNOAA Thank you, Sam, for checking. In my opinion the changes to GFS_diagnostics.F90 should stay. The only change I can do in GFS_diagnostics.F90 is not to output surface fire heat flux and ratio of burned area. I can put 'if(Model%add_fire_heat_flux) then' around that output, and the default of add_fire_heat_flux is .false. |
|
Also, this fire_heat_flux is happening only with smoke physics on. |
I'm not suggesting removing the changes in GFS_diagnostics, I'm just wanting to understand whether these code changes in totality are doing what you expect without changing something that you didn't expect. |
|
@tanyasmirnova @SamuelTrahanNOAA So far, it seems like the only files that are listed as "NOT OK" in the non-HRRR/RAP tests are the GFSPRS.GrbFXX files. I don't know what is being written there. |
This probably means a field that is not in the diagnostic files changed. |
Please try merging the top of the authoritative branches. It's possible the UPP failure is unrelated to Tanya's changes. I've run into many UPP bugs where an out-of-bounds read causes results to change or a test to fail after changing FV3 in unrelated ways. The last PR that was merged had a new version of UPP. With luck, this will fix our problems. |
Ya, I saw that UPP was just updated. The original RTs that I did were with updates to the latest develop at the time (last Friday or so). I'm going to try 2 things. I'll run one of the unexpected failures with the top of develop against the stored baselines. If that passes, I'll run the same individual test with the latest develop code merged in. If that passes, I'll kick off another round of all tests with the latest develop code merged in. |
|
@tanyasmirnova @SamuelTrahanNOAA A quick update -- I think that there is good news. One of the unexpected tests that had been failing is now passing with the latest develops merged in. I'm guessing that there was something amiss with upp that was fixed in the latest commit. I'm now running all tests and will post results tomorrow. |
The plan was to combine this with #1945. Will you be able to do that? |
Yes. I didn't want to combine them until each was exhibiting the expected behavior individually, though. |
|
OK, I think that we're in business. Here is the list of failed RTs after updating the code to the latest develops. These are all expected, I think. I'll go ahead and merge this into Joe's branch for final testing and merge. 063 rap_control_intel failed in check_result |
|
@grantfirl This is showing as having conflicting FV3 hashes. Do we know for sure the test was w/ an up-to-date FV3? |
Yes. I manually updated all branches on Hera before kicking off the tests. I didn't push the updates to Tanya's branch because I don't think that I have permissions, and I'm merging this into #1945 right now anyway. |
|
@grantfirl Thank you for you work and good news. |
|
This PR has been combined with #1945. |
| @@ -0,0 +1,1807 @@ | |||
| + SECONDS=0 | |||
There was a problem hiding this comment.
This file should not be committed.
| @@ -0,0 +1,27 @@ | |||
| help([[ | |||
There was a problem hiding this comment.
This file should not be committed.
| @@ -0,0 +1,229 @@ | |||
| ### RT.CONF FORMATTING ### | |||
There was a problem hiding this comment.
This file should not be committed
| @@ -352,8 +352,11 @@ elif [[ $MACHINE_ID = hera ]]; then | |||
| PARTITION= | |||
| dprefix=/scratch1/NCEPDEV | |||
| DISKNM=/scratch2/NAGAPE/epic/UFS-WM_RT | |||
| STMP=$dprefix/stmp4 | |||
| PTMP=$dprefix/stmp2 | |||
| #STMP=$dprefix/stmp4 | |||
There was a problem hiding this comment.
These need to be reverted to the standard locations.
|
@DeniseWorthen All of the extra files and edited rt.sh were removed when I merged this into #1945. I was assuming that this PR would be closed as soon as #1945 is merged. |
|
@grantfirl Thanks. I wasn't sure which PR was the receiving PR for both sets of changes. |
|
Merged with #1945 |
PR Author Checklist:
Description
Linked Issues and Pull Requests
Associated UFSWM Issue to close
Closes ufs-community/ccpp-physics#117
Subcomponent Pull Requests
### Blocking Dependencies
Subcomponents involved:
Anticipated Changes
Input data
Regression Tests:
Tests effected by changes in this PR:
Libraries
RUC LSM has a modification that will change the results with the use of RUC LSM.
Code Managers Log
Testing Log: