-
Notifications
You must be signed in to change notification settings - Fork 61
bugfix for reading in multi-level unstructured data plus additional cleanup #368
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
bugfix for reading in multi-level unstructured data plus additional cleanup #368
Conversation
jedwards4b
left a comment
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 couple of minor comments, but this looks good overall. Thank you.
| new_lines.append(new_line) | ||
| else: | ||
| print(f" WARNING:not adding missing file {new_file}") | ||
| print(f" WARNING:not adding missing file {new_line}") |
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.
Should this message be changed from "missing file" to "missing line"?
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.
It actually should be new_line - where new_line actually represents the file that is looked for. I can try to update this more generally in the next cdeps PR coming up soon.
dlnd/lnd_comp_nuopc.F90
Outdated
| integer :: mpicom ! mpi communicator | ||
| integer :: my_task ! my task in mpi communicator mpicom | ||
| logical :: mainproc ! true if my_task == main_task | ||
| logical :: mainproc ! true of my_task == main_task |
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.
Should be 'true if' not 'true of'
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.
done
billsacks
left a comment
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.
Thank you very much for all of this great cleanup, @mvertens ! Thank you also for your detailed description of the changes in the PR comment.
I have a number of minor questions and fixes below, but overall this looks great!
| call shr_lnd2rof_tracers_readnl('drv_flds_in', lnd2rof_tracers) | ||
| if (lnd2rof_tracers /= ' ') then | ||
| ntracers_nonh2o = shr_string_listGetNum(lnd2rof_tracers) | ||
| if (ntracers_nonh2o > 99) 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 appreciate this error check, but I'm assuming that this 99 corresponds to a hard-coded 99 somewhere else? If so, can you turn this into a parameter that is shared between these different locations?
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.
oGood point. I have introduced a new module variable - ntracers_nonh2o_max.
|
@billsacks - thanks so much for your review and comments. I believe I have addressed all of them. |
billsacks
left a comment
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.
Looks good now - thank you very much! I do have one comment/question below, but I'm happy to approve this regardless. Thanks again!!
billsacks
left a comment
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.
As we just discussed: can you please just add a comment describing why there's a limit of 99 tracers?
Done. |
billsacks
left a comment
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 - looks good now!
…l_streams2
new datm optional streams
### Description of changes
This introduces the following new stream modules that are optional and can be added to the datamode streams already in place:
### Specific notes
The following new modules have been added
- datm_pres_aero_mod.F90, datm_pres_co2_mod.F90, datm_pres_ndep_mod.F90, datm_pres_o3_mod.F90
For datm_pres_ndep_mod.F90:
- added new streams clim_1850_cmip7, clim_2000_cmip7, clim_2010_cmip7, and hist_cmip7 (currently these are only used by NorESM)
- renamed the following streams
- clim_1850 -> clim_1850_cmip6
- clim_2000 -> clim_2000_cmip6
- ciim_2010 -> clim_2010_cmip6
- hist -> hist_cmip6
by default for CESM, the following cmip6 streams will be used (whereas for NorESM, the cmip7 streams are used)
Note that for the cmip7 data, no unit conversion is done whereas for the default cmip6 data the input data is converted from` g/m2/sec` to `kg/m2/sec`.
For datm_pres_co2_mod.F90:
- added the following logic which will change answers for non-CPLHIST mode
```F90
if (datamode == 'CPLHIST') then
Sa_co2diag(:) = strm_Sa_co2diag(:)
Sa_co2prog(:) = strm_Sa_co2prog(:)
else
! This is intentional since we don't have any Sa_co2prog - but for now
! will set Sa_co2prog equal to Sa_co2diag
Sa_co2diag(:) = strm_Sa_co2diag(:)
Sa_co2prog(:) = strm_Sa_co2diag(:)
end if
```
Also
- removed references to water isotopes in DATM modules since this will be refactored in upcoming work.
- removed style sheet references in all component namelist_definition_dXXX.xml and stream_definition_dXXX.xml since they are not used.
Contributors other than yourself, if any: None
CDEPS Issues Fixed: #370
The following tests now have correct values of o3 sent to
```
ERI_D_Ld11.ne30pg3_ne30pg3_mtn14.I1850Clm60FatesNocompSpinup.betzy_gnu.clm-FatesColdSpinupAD
ERS_D_Ld11.ne30pg3_ne30pg3_mtn14.I1850Clm60FatesNocompSpinup.betzy_intel.clm-FatesColdSpinup
SMS_D_Ld1000_P1536.ne30pg3_ne30pg3_mtn14.I1850Clm60FatesNocompSpinup.betzy_gnu.clm-FatesColdSpinup
```
Are there dependencies on other component PRs: This PR contains all the changes from CDEPS PR #368.
Are changes expected to change answers (bfb): bfb except for Sa_o3 bug fix and when DATM_PRES_CO2 is used.
Any User Interface Changes: add new options for DATM_PRESNDEP
Testing performed: Successfully carried out aux_cdeps_noresm, aux_blom_noresm and aux_clm_noresm with a corresonding PR in NorESMhub (see testing description in NorESMhub#27).
Hashes used for testing:
Description of changes
This PR fixes a CDEPS bug that was found using stream input ne16pg3 with 58 vertical levels using NorESM. CDEPS aborted trying to read this in.
Specific notes
This PR also does the following:
character(*) with character(len=*)logunitandmainprocin shr_strdata_type and bothlogunitandmainprocin shr_stream_streamType. This is needed since the inline interface was not always writing all log output consistently.dshr_strdata_mod.F90forshr_strdata_get_stream_pointer_1d and shr_strdata_get_stream_pointer_2d
and
If requirePointer is not provided - then if the pointer is not found, the subroutine returns without an error. If requirePointer is an argument and is true, than normally an errmsg is provided that describes why the pointer is required. Also - now if the pointer is required and is found - then the pointer is initialized to NaN.
The new setting of NaNs in the stream and state pointers resulted in the following additional changes that needed to be brought in:
Contributors other than yourself, if any: None
CDEPS Issues Fixed:
Are there dependencies on other component PRs: None
Are changes expected to change answers (bfb, different to roundoff, more substantial): bfb
Any User Interface Changes (namelist or namelist defaults changes): None