-
Notifications
You must be signed in to change notification settings - Fork 61
new datm optional streams #373
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
new datm optional streams #373
Conversation
…with a vertical dimension
…ew_datm_optional_streams
…am_pointer_2d to have optional arguments requirePointer and errmsg and set default values to nan
…lev_input' into feature/new_datm_optional_streams
…ort field pointers
…ams' into feature/escomp_new_datm_optional_streams
…escomp_new_datm_optional_streams2
|
@billsacks - this new PR no longer has the changes from #368. I'm not sure what happened - but I could not get github to recognize what I was seeing in terms of my changes in my local directory. |
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 this work, @mvertens ! This is a very nice cleanup that removes some existing duplication and will make it easier to keep things in sync moving forward, e.g., if new datm modes are added that need these various shared streams.
I have one thought for a possible improvement below, but I see it as optional, so I'm marking this as approved and will let you decide if my thought seems worth doing.
|
@fischer-ncar - I'm adding you as a reviewer here so you can formally approve once your testing (aux_cdeps?) looks good. @jedwards4b - do you want to review this before we merge it? (No rush/pressure, just wondering if we should wait for your review.) |
Oh, I just saw the email from yesterday describing the testing - so no need for any further comments / approval, Chris. Here is Chris's message for the record:
|
|
I'm removing @jedwards4b as reviewer since he's out of the office this week, and he approved the earlier iteration of this (#369 ) |
The line break after the last valid value (cplhist) led it to be interpreted as "cplhist\n", so "cplhist" was determined to be not a valid value. So apparently we can't have a newline after the last valid value; I'm not sure about having newlines in other places, but it looked funny to have newlines in other places and not after the last one, so I'm just removing all newlines here. This reverts a change that was made as part of ESCOMP#373
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
For datm_pres_ndep_mod.F90:
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/sectokg/m2/sec.For datm_pres_co2_mod.F90:
Also
Contributors other than yourself, if any: None
CDEPS Issues Fixed: #370
The following tests now have correct values of o3 sent to
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: