land surface upgrades for HR2 #78
Conversation
sync with the new community repository
UFS/dev PR#6
Add tests for ccpp_prebuild step
UFS/dev PR#1
UFS-dev PR#10
UFS-dev PR#14
UFS-dev PR#19
sync with the ufs/dev branch
sync with the latest ufs/dev branch
sync with the develop
sync with emc develop
update main with the develop
sync with the main
| irtsoc=iret | ||
| if(iret.eq.1) then | ||
| write(6,*) 'FATAL ERROR: soil color analysis read error.' | ||
| call abort |
There was a problem hiding this comment.
I notice a bunch of abort calls throughout. CCPP schemes are not supposed to be able to kill the model. They're supposed to set the errflg and errmsg variables and return so that the host can exit gracefully.
| @@ -7394,6 +7546,7 @@ subroutine clima(lugb,iy,im,id,ih,fh,len,lsoil,slmskl,slmskw, & | |||
| endif | |||
| ! | |||
| ! soil type | |||
| print *,'in FIXREAD fnsotc =',fnsotc | |||
There was a problem hiding this comment.
same comment about print lines
There was a problem hiding this comment.
@grantfirl all your comments about sfcsub.F were not introduced by this PR. If we want to use this opportunity to make these changes, we should consult POC of the subroutine @GeorgeGayno-NOAA
There was a problem hiding this comment.
Although some of the comments were referring to pre-existing aborts, most of the comments refer to new print statements added in this PR. People complain about command line output being "polluted". I understand the developer need to have print lines in the code, but they should not be active by default. @GeorgeGayno-NOAA what do you think?
There was a problem hiding this comment.
Yes, you can get rid of the print line.
@dustinswales Please re-review since you requested changes and blocked merging. |
sync with the ufs/dev
sync with the ufs/dev
|
I know that there are approvals here, but since there have been code changes since the latest approval, can we get a re-approval from @dustinswales or @Qingfu-Liu of those changes? |
|
@BrianCurtis-NOAA new changes look good, should rerun the regression tests |
|
@jkbk2004 It looks like this PR was not updated to the latest ufs/dev branch after the merge of #85. This shouldn't necessarily matter for testing because that PR only changed 2 files that are immaterial to the changes in this PR, but it is a mistake not to have updated this branch before final testing, IMO. I can still merge if we're all OK with it, I'm just pointing it out since I just got back from vacation. |
|
I would prefer that the code/hashes that go into the UFSWM repo be tested, so if there's a mistake in the upstream UFSWM testing branch it should be corrected and retested. Please do not merge. |
|
We can do a quick test to check the impact. BTW, we should hold this pr a bit. |
|
@jkbk2004 @BrianCurtis-NOAA False alarm, I guess. I was looking through the commits and didn't see the merge commit for #85, but it looks like e9803fc did this. It should be OK to merge whenever you're ready. |
OK good. I was getting frustrated why I couldn't see the discrepancy you had mentioned and was about to ask if you were certain of it. |
|
Ok, in that case it looks like this should be good to go. Testing for #1777 has finished successfully, please continue with the merge process. |
See this PR for reference: ufs-community/ccpp-physics#78
land surface upgrades for HR2: