-
Notifications
You must be signed in to change notification settings - Fork 111
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.
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
Add capability to read soil increments on the cubed-sphere tiles and enhance soil ice adjustment due to soil temperature increments #894
Conversation
Please re-review, @GeorgeGayno-NOAA and @ClaraDraper-NOAA, Thank you for your help! |
I placed the input data - soil_xainc.00? - for one of your regression tests under the official test directory. You can now update
|
@yuanxue2870 The latest merge to |
Thanks for the heads up, and thanks for your help in advance! |
Done. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My wifi cutout, and I thought I'd lost my review and started again - but now I think it's still there. Ignore this for now, and give me a minute to clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a look at all of it, except the apply_increments file, as there are too many changes for git to be helpful. I'll check it out now, and compare them.
So far, my main comments are:
- This is a very large PR, which makes it very difficult to review.
- I'm not clear that your "JEDI" soil increment files are actually from JEDI (as opposed to being increment files you created on the native model grid). If they're not from JEDI, we'll need to create some, and check that this code works with them. I can help with that.
@@ -1512,6 +1577,22 @@ SUBROUTINE READ_DATA(LSOIL,LENSFC,DO_NSST,INC_FILE,IS_NOAHMP, & | |||
STCFCS = RESHAPE(DUMMY3D, (/LENSFC,LSOIL/)) | |||
ENDIF | |||
|
|||
IF (PRESENT(SLCINC)) THEN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm 99% sure that the files created by JEDI use the variable name for the increment (not "*_inc").You can delete the SLCINC and STCINC options, and just use the STC and SLC options above (there's no special snow increment option, which makes me think I'm right).
If you have done this, and developed the tests using files you created yourself (for synthetic experiments), then I think we need to generate some actual JEDI files and use those instead. I can help with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this issue with Clara via email on 02/23/2024. For now, the soil_xainc.00$N files are created using the output from interpolated GSI increments. Will contact Mike to create some real-JEDI derived increments.
@@ -85,6 +93,51 @@ subroutine set_soilveg(isot,ivet, maxsmc, bb, satpsi, iret) | |||
|
|||
iret = 0 | |||
|
|||
end subroutine set_soilveg | |||
end subroutine set_soilveg_noah |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barlage Don't suppose there's a better way to do this (read them in from the same place the model does). Or are they very unlikely to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments on changes to land_increments.
Sorry for mutiple reviews - it's a big PR.
n_stc = 0 | ||
n_slc = 0 | ||
|
||
do i=1,lensfc | ||
if (stc_updated(i) == 1 ) then | ||
if (stc_updated(i) == 1 ) then ! soil-only location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gone through the logic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mike @barlage, Clara mentioned that she would like you to review this part of the code (i.e., consistency check). Thanks!!
I think I cleaned everything up. Thanks @yuanxue2870 for all your hard work on this. It's coming together! |
One more thought. Instead of using a single routine to interpolate (if needed), then add the increments - maybe it'd be better to have one routine for interpolating (if needed), then one for adding. |
Feel free to break up this work into multiple pull requests. |
@KateFriedman-NOAA - you had hosted the Could you please replace them with the |
@GeorgeGayno-NOAA Done:
|
@yuanxue2870 I was able to successfully run the unit tests with the new files Kate hosted. I had to make these modifications:
|
Thanks. As Kate helps us hosting the increment files, I have now made edits to the unit test file reader and the ush/global_cycle_driver file to link directly from the public host. Your solution is also viable, but if we modify the filename in the main read_write.f90 as such, it will not be consistent with snow JEDI increment files then. I guess one of the main purposes of this PR is to make soil and snow related names/routines consistent. Therefore, I would like to retain the filename as "soil_xainc" since it is consistent with "snow_xainc". Please feel free to let me know if there are any further concerns. I am happy to adjust if needed. |
Your changes at 28a8cef are fine. |
@@ -263,7 +264,9 @@ use_ufo=${use_ufo:-.true.} | |||
DONST=${DONST:-"NO"} | |||
DO_SFCCYCLE=${DO_SFCCYCLE:-.true.} | |||
DO_LNDINC=${DO_LNDINC:-.false.} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DO_SOI_INC_GSI is not defined in the prolog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi George, thanks for the question. What do you mean by "prolog"? DO_SOI_INC_GSI along with other two options were defined in the ush/global_cycle.sh from Lines 267-269:
267 DO_SOI_INC_GSI=${DO_SOI_INC_GSI:-.false.}
268 DO_SNO_INC_JEDI=${DO_SNO_INC_JEDI:-.false.}
269 DO_SOI_INC_JEDI=${DO_SOI_INC_JEDI:-.false.}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define it after line 144.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done.
@@ -472,6 +482,47 @@ subroutine write_data(lensfc,idim,jdim,lsoil, & | |||
call remove_checksum(ncid, id_var) | |||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are writing a new file - gaussian_interp.* - you could have created a new write routine to do this. Just a comment. You don't need to do this.
@ClaraDraper-NOAA and @barlage - I would like to merge this. Do you have any further comments? @yuanxue2870 - please merge the latest updates from 'develop' to your branch. |
@yuanxue2870 - I will need updated baseline files for regression tests 2 and 5. I am currently using:
|
Done. Merged the latest updates from develop now. |
My regression tests output are located on Hera at /scratch2/NCEPDEV/stmp1/Yuan.Xue/reg-tests/global-cycle/test2 for c192.gsi_lndinc and at /scratch2/NCEPDEV/stmp1/Yuan.Xue/reg-tests/global-cycle/test5 for c192.jedi_lndinc, respectively. |
For test2, the 'gaussian_interp.00?' files are outputs, correct? If so, the regression test should compare them to the baseline. And the regression test script logic will need to be updated. |
Added. Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look through, and my comments have been addressed. I haven't done a very in depth review though.
Well done @yuanxue2870
@yuanxue2870 - looks good. Will merge. |
DESCRIPTION OF CHANGES:
calculate_landinc_mask
to make it more stable_JEDI
to the originalDO_SNO_INC
option for snow increment to be consistent with newly-added JEDI-based soil increment option ofDO_SOI_INC_JEDI
DO_SOI_INC_GSI
and remoteGSI_SOI_FILE
option in the namelistlsoil_incr
and set it to 3 as the default valueftst_read_increments
) to read jedi-based soil increments on the cubed-sphere grid. This unit test requires input data, these input data will be housed elsewhere, other than Github. George is coordinating with Kate to host the test data onlineftst_land_increments
) to ingest soil temperature increments and adjust soil ice content using newly-revised schemeC192.jedi_lndincsoilnoahmp.sh
) to ingest jedi-based soil increments and apply adjustment. This newly-added regression test requires the same input data from the unit test. The original regression test#2 was renamed fromC192.lndincsoilnoahmp.sh
toC192.gsi_lndincsoilnoahmp.sh
TESTS CONDUCTED:
If there are changes to the build or source code, the tests below must be conducted. Contact a repository manager if you need assistance.
All my code edits should only affect global_cycle related unit tests and regression tests:
REFERENCES:
This PR incorporates all reviews/feedback from PR#871 (#871))
ISSUE:
Fixes #872.
NOTES:
apply_land_da_adjustments_soil
routine,C192.jedi_lndincsoilnoahmp.sh
andC192.gsi_lndincsoilnoahmp.sh
yields identical results. With theapply_land_da_adjustments_soil
routine,C192.jedi_lndincsoilnoahmp.sh
andC192.gsi_lndincsoilnoahmp.sh
yields slightly different results on tile3 because GSI option checks nearby soil and no updates will be taking place for GSI grid with no soil nearby. However, JEDI option does not check nearby soil.