-
Notifications
You must be signed in to change notification settings - Fork 61
major refactor stream of stream usage #376
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
base: main
Are you sure you want to change the base?
major refactor stream of stream usage #376
Conversation
…mode specific streams
…d_stream_usage' into feature/refactor_stream_usage
…t field pointers to nans
…ams' into feature/refactor_stream_usage
…efactor_stream_usage
…al_streams' into feature/refactor_stream_usage
|
@NickSzapiro-NOAA - I have refactored dice_datamode_cplhist_mod.F90 and verified that it compiles - but I cannot test it in either CESM or NorESM. If you could verify that this is working for UFS that would be great. Also - do you run docn in CPL hist mode? If not - I am wondering what the use case for it is - since I really cannot see how it would be used in either CESM or NorESM. |
|
Just to record what I talked about with @mvertens
|
|
@billsacks - I have added a new datamode in drof - drof_datamode_cplhist.F90 - along with expanding the set of stream fields needed in cplhist mode. I have added a new testmods directory - cplhist_noresm and a new test that will now test this new functionality. I think we should create a new parallel directoyr cplhist_cesm that points the relevant datafiles that are needed to test this for cesm. All that is needed is a new shell_commands file. |
drof/drof_datamode_copyall.F90
Outdated
| if (associated(strm_Forr_rofi)) then | ||
| do ni = 1, size(Forr_rofi) | ||
| if (abs(strm_Forr_rofi(ni)) < 1.e28_r8) then | ||
| Forr_rofi(:) = strm_Forr_rofi(:) |
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.
Forr_rofi(ni) = strm_Forr_rofi(ni) instead of rofi(:)
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.
Ooops - thanks so much for catching 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.
Fixed.
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.
@mvertens - you commented that this is fixed (changing the whole-array assignment of Forr_rofi to just Forr_rofi(ni)), but it looks like you haven't yet pushed a commit fixing this? (You fixed other, similar issues.)
drof/drof_datamode_cplhist.F90
Outdated
|
|
||
| do ni = 1, size(Forr_rofl_glc) | ||
| if (abs(strm_Forr_rofl_glc(ni)) < 1.e28_r8) then | ||
| Forr_rofl_glc(:) = strm_Forr_rofl_glc(:) |
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.
Forr_rofl_glc(ni) = strm_Forr_rofl_glc(ni) instead of glc(:)
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.
Fixed.
drof/drof_datamode_cplhist.F90
Outdated
|
|
||
| do ni = 1, size(Forr_rofi) | ||
| if (abs(strm_Forr_rofi(ni)) < 1.e28_r8) then | ||
| Forr_rofi(:) = strm_Forr_rofi(:) |
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.
Forr_rofi(ni) = strm_Forr_rofi(ni)
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.
fixed.
drof/drof_datamode_cplhist.F90
Outdated
|
|
||
| do ni = 1, size(Forr_rofi_glc) | ||
| if (abs(strm_Forr_rofi_glc(ni)) < 1.e28_r8) then | ||
| Forr_rofi_glc(:) = strm_Forr_rofi_glc(:) |
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.
Forr_rofi_glc(ni) = strm_Forr_rofi_glc(ni)
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.
Fixed.
datm/datm_datamode_cplhist_mod.F90
Outdated
| call shr_strdata_get_stream_pointer(sdat, 'Sa_dens', strm_Sa_dens, requirePointer=.true., & | ||
| errmsg=subname//'ERROR: strm_Sa_ndens must be associated for cplhist datamode', rc=rc) | ||
| if (ChkErr(rc,__LINE__,u_FILE_u)) return | ||
| call shr_strdata_get_stream_pointer(sdat, 'Sa_pslv', strm_Sa_pslv, requirePointer=.true., & | ||
| errmsg=subname//'ERROR: strm_Sa_pslv must be associated for cplhist datamode', rc=rc) | ||
| if (ChkErr(rc,__LINE__,u_FILE_u)) return | ||
| call shr_strdata_get_stream_pointer(sdat, 'Sa_dens', strm_Sa_dens, requirePointer=.true., & | ||
| errmsg=subname//'ERROR: strm_Sa_dens must be associated for cplhist datamode', rc=rc) | ||
| if (ChkErr(rc,__LINE__,u_FILE_u)) return |
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.
Sa_dens is here 2x
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 for catching this. Fixed.
|
Thanks @mvertens ! dice cplhist test is ok in UFS with refactor UFS RTs pass, except for datm_cdeps_lnd_era5_intel and its restart. These fail in run with Especially since this was PR from @uturuncoglu in ufs-community/ufs-weather-model#1845 , tagging here. Currently in this test, I think tdew needs to be calculated in datm/datm_datamode_era5_mod.F90 |
For UFS, I think docn cplhist and sstdata can be the same, if can add units_CToK |
|
@NickSzapiro-NOAA @mvertens I wonder if there is a way to get rid of lsize completely such as querying mesh dimensions etc. It would be better solution and stream will not depend on any particular variable. |
|
@NickSzapiro-NOAA- thanks for your testing! @NickSzapiro-NOAA @uturuncoglu - I think I am missing something in datm_datamode_era5_mod.F90. In the the original code for this: if (first_time)
! determine t2max (see below for use)
if (associated(Sa_t2m)) then
rtmp(1) = maxval(Sa_t2m(:))
call ESMF_VMAllReduce(vm, rtmp, rtmp(2:), 1, ESMF_REDUCE_MAX, rc=rc)
t2max = rtmp(2)
if (mainproc) write(logunit,*) subname,' t2max = ',t2max
end if
! determine tdewmax (see below for use)
rtmp(1) = maxval(strm_tdew(:))
call ESMF_VMAllReduce(vm, rtmp, rtmp(2:), 1, ESMF_REDUCE_MAX, rc=rc)
td2max = rtmp(2)
if (mainproc) write(logunit,*) subname,' td2max = ',td2max
! reset first_time
first_time = .false.
endif
In the new code:
What does the ESMF configuration file look like for this datamode? |
|
@NickSzapiro-NOAA It would be also nice to see actual error. |
|
@NickSzapiro-NOAA We have just have a short chat with @mvertens about the issue. It seems that UFS WM is using CDEPS develop branch (https://github.com/NOAA-EMC/CDEPS/blob/develop/datm/datm_datamode_era5_mod.F90) and the ERA5 mode in that branch is different from the one found in ESCOMP. So, that was the issue. @mvertens will check the new code found in develop branch and try to fix the issue in ERA5 mode. Then once this PR merged, UFS WM needs to sync with ESCOMP to bring these changes. |
Great, thank you for clarifying that. |
@mvertens - Great, thanks! If the purpose of this is software testing, would it be reasonable for CESM to use the NorESM cplhist files for testing, so that we're running consistent tests and don't need to do double-maintenance on this test? |
|
@mvertens @billsacks is this PR about ready for prealpha testing? |
drof/drof_datamode_copyall.F90
Outdated
| if (associated(strm_Forr_rofi)) then | ||
| do ni = 1, size(Forr_rofi) | ||
| if (abs(strm_Forr_rofi(ni)) < 1.e28_r8) then | ||
| Forr_rofi(:) = strm_Forr_rofi(:) |
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.
@mvertens - you commented that this is fixed (changing the whole-array assignment of Forr_rofi to just Forr_rofi(ni)), but it looks like you haven't yet pushed a commit fixing this? (You fixed other, similar issues.)
|
@billsacks - I have pushed the drof change back. |
It looks like you changed one of the |
Oh, never mind - I see your second commit. |
|
@fischer-ncar - thanks for checking in. I talked to @mvertens and we both feel this is ready for your testing. See #376 (comment) for some notes on testing - specifically a few tests to add beyond the standard prealphas on derecho. I'm in the midst of a deep dive into the different data model modes to make sure we're running testing covering everything we want to test, so I may come up with a few additional tests in the next day or so, but the prealphas plus the tests noted above should be close to what we want. Thank you! |
|
@fischer-ncar and @mvertens - I have done a pretty detailed analysis of the different data modes and their coverage in prealpha testing. In addition to the CESM prealpha testing, I want to make sure the following are covered: @fischer-ncar - can you please add the following two tests to your testing, along with baseline comparisons (these tests are present in the aux_mom or aux_cice test lists but will need baselines for your testing) (I have also updated my testing comment with this request so it's all in one place):
@mvertens - I don't think we have a way to test the following in CESM, so I wanted to double-check that you have tested these (sorry, you might have answered this before):
@mvertens - also, I think you said you have tested DWAV... just wanted to double-check that, since we don't have any DWAV testing in our prealpha test list (there's a test of ADWAV in aux_cmeps). (EDIT: I see that you ran aux_cdeps_noresm, which covers this.) |
Description of changes
Major refactor of stream usage
Specific notes
All data mode files across the data components(except for
datm_datamode_gefs_mod.F90anddice_datamode_cplhist_mod.F90) now have explicit setting of streams rather than using the implicit copy used bydfields. Copies that used to be done implicitly using the naming convention assumed by dfields arenow done explictly. This makes it clear what export fields are direct copies and what export fields are
derived fields that have no corresponding stream field.
In each datamode module - there are now two sections in the module variable list:
export (and sometimes import) state pointersandstream pointer.The following changes have been made throughout the code:
character(*) => character(len=*),trim(subname) => subname(the latter is done since parameters, e.g.subname, don't need trim functions)The following new routines have been introduced or deleted:
drof_datamode_cplhist_mod.F90) to support this addition.docn_datamode_copyall_mod.F90anddocn_datamode_iaf_mod.F90into a new filedocn_datamode_sstdata_mod.F90. The two filesdocn_datamode_copyall_mod.F90anddocn_datamode_iaf_mod.F90were effectively the same and two different datamodes are not needed since in both cases prescribed SST data was read in. (In addition, docn_datamode_iaf_mod.F90` set pointers to importState data which was never used and not needed).NOTE: This PR also provides a new mapfile algorithm as a new option for mapping the streams input to the component model resolution. The new scheme does not have a default but is enabled via adding the following in the appropriate user_nl_XXX_streams where the user needs to fill in the entries in <> below:
<stream_name>:mapalgo = "mapfile:
As an example:
rof.ryf8485_jra:mapalgo="mapfile:/cluster/shared/noresm/inputdata/cpl/cpl6/map_JRA025_to_tnx1v4_e1000r300_170928.nc". This new capability enables a significant speedup in stand-alone ocean simulations. See NorESMhub/BLOM#686 (comment).NOTE*:
Contributors other than yourself, if any: None
CDEPS Issues Fixed: #377
Are there dependencies on other component PRs (if so list):
Are changes expected to change answers (bfb, different to roundoff, more substantial):
Any User Interface Changes (namelist or namelist defaults changes):
Testing performed (will describe the noresm testing here)
Hashes used for testing: noresm3_0_beta09 plus this CDEPS branch