refactor CPLHIST mode and add DATM CPLHIST topo capability #1950

mvertens · 2017-10-04T17:46:55Z

This PR has several features:

unifies the CPLHIST naming convention across the components by
adding the following new xml variables in DATM, DLND and DOCN
DATM_CPLHIST_DOMAIN_FILE, DLND_CPLHIST_DOMAIN_FILE, DROF_CPLHIST_DOMAIN_FILE
These new xml variables are the full pathname for domain file
for datm, dlnd and drof when the corresponding data model mode
is set to CPLHIST.
NOTE: if any of these xml variables set to 'null',
then domain information is read in from the first coupler
history file in the target stream.
NOTE: it is assumed that when DXXX_CPLHIST_DOMAIN_FILE='null'
that the first coupler stream file that is pointed
to contains the domain information for that stream.
NOTE: This is the default mode that should be used when the data model
DXXX_MODE is CPLHIST.
puts in new capability for DATM CPLHIST mode to read in topo data
replaces % formatting in the data models with new .format()
address issue Review mode implementation in data models #1877
replaced xxx_mode (e.g. atm_mode) variables in dxxx_comp_mod.F90 and dxxx_shr_mod.F90 with datamode - to reduce confusion.

Additional testing:
Verified that using this CIME branch to replace cime/ in cesm2_0_alpha07d,
the following cases ran successfully on cheyenne with data that was already there:

1850_DATM%CPLHIST_CLM50%BGC_SICE_SOCN_MOSART_CISM2%NOEVOLVE_SWAV (f09_f09_mg17)
   the following xmlchanges neededed to be made:
   ./xmlchange DATM_CPLHIST_DOMAIN_FILE=null
   ./xmlchange DATM_CPLHIST_DIR=/glade/p/cesm/bgcwg_dev/forcing/b.e20.B1850.f09_g17.pi_control.all.179.cplhist/cpl/hist.mon
   ./xmlchange DATM_CPLHIST_CASE=b.e20.B1850.f09_g17.pi_control.all.179.cplhist
   ./xmlchange DATM_CPLHIST_YR_ALIGN=1
   ./xmlchange DATM_CPLHIST_YR_START=37
   ./xmlchange DATM_CPLHIST_YR_END=37

1850_DATM%CPLHIST_SLND_CICE_POP2%ECO_DROF%CPLHIST_SGLC_WW3 (f09_g17)
   the following xmlchange commands needed to be run:
   ./xmlchange DATM_CPLHIST_DOMAIN_FILE=null
   ./xmlchange DATM_CPLHIST_DIR=/glade/p/cesm/bgcwg_dev/forcing/b.e20.B1850.f09_g17.pi_control.all.179.cplhist/cpl/hist.mon
   ./xmlchange DATM_CPLHIST_CASE=b.e20.B1850.f09_g17.pi_control.all.179.cplhist
   ./xmlchange DATM_CPLHIST_YR_ALIGN=1
   ./xmlchange DATM_CPLHIST_YR_START=37
   ./xmlchange DATM_CPLHIST_YR_END=37
   ./xmlchange DROF_CPLHIST_DOMAIN_FILE=null
   ./xmlchange DROF_CPLHIST_DIR=/glade/p/cesm/bgcwg_dev/forcing/b.e20.B1850.f09_g17.pi_control.all.179.cplhist/cpl/hist.mon
   ./xmlchange DROF_CPLHIST_CASE=b.e20.B1850.f09_g17.pi_control.all.179.cplhist
   ./xmlchange DROF_CPLHIST_YR_ALIGN=1
   ./xmlchange DROF_CPLHIST_YR_START=37
   ./xmlchange DROF_CPLHIST_YR_END=37

1850_SATM_DLND%SCPL_SICE_SOCN_SROF_CISM2%EVOLVE_SWAV (f09_g16_gl20)
    no xmlchange commands were needed since in this case we have
      DLND_CPLHIST_DOMAIN_FILE" value="$LND_DOMAIN_PATH/$LND_DOMAIN_FILE"
    which is obtained from the config_grids.xml file

Test suite: scripts_regression_tests
Test baseline: did not compare against baselines
Test namelist changes: None
Test status: should be bfb

Fixes #1877
Fixes #1407

User interface changes?: added new xml variables discussed above
Update gh-pages html (Y/N)?: Y
Code review:

…put to be consistent with python3

gold2718 · 2017-10-04T19:54:48Z

doc/source/data_models/data-atm.rst

+   "DATM_CLMNCEP_YR_START",    "I compsets only - data model starting year to loop data over"
+   "DATM_CLMNCEP_YR_END",      "I compsets only - data model ending year to loop data over"
+
+.. note:: If ``DATM_MODE`` is set to ``CPLHIST``, it is normally assumed that the model domain will be identical to **all** of the stream domains. To ensure this, the xml variables ``ATM_DOMAIN_PATH`` and ``ATM_DOMAIN_FILE`` are ignored and a valid setting **must be given** for ``DATM_CPLHIST_DOMAIN_FILE``. If ``DATM_CPLHIST_DOMAIN_FILE`` is set to ``null``, then the datm component domain information is read in from the first coupler history file in the target stream and  it is assumed that the first coupler stream file that is pointed to contains the domain  information for that stream. This is the default mode that should be used for this mode. Alternatively, ``DATM_CPLHIST_DOMAIN_FILE`` can be set to ``$ATM_DOMAIN_PATH/$ATM_DOMAIN_FILE`` in a non-default configuration.


In another discussion, it was thought that 'none' is more user accessible than 'null'. Is it possible to use 'none' in this context?

Unfortunately, this is not really possible in this case. A 'null' value is hard-wired as the default value in the data model shr strdata input namelist - and it would be good to be consistent with the code. This could be changed -but I'm really wary to do this now.

gold2718 · 2017-10-04T19:59:29Z

Is #1877 'fixed' by this change? If so, please add it to the 'Fixes' section of the header.

billsacks · 2017-10-04T22:34:36Z

#1407 is fixed by this, right? So it should be added to the "Fixes" section.

billsacks · 2017-10-04T22:44:30Z

Minor documentation point: In your big initial comment, you have three blocks showing the tests you ran. The second is missing information on what compset that's referring to.

billsacks · 2017-10-04T22:46:42Z

From your documentation in this PR, it looks like you need to explicitly set ./xmlchange DATM_CPLHIST_DOMAIN_FILE=null? From our earlier discussions, I thought this was going to be the out-of-the-box default?

mvertens · 2017-10-04T22:57:23Z

@billsacks - It is the out of the box default - but its always good to explicitly state it for consistency of specifying what xmlchange commands are needed.

…

On Wed, Oct 4, 2017 at 4:46 PM, Bill Sacks ***@***.***> wrote: From your documentation in this PR, it looks like you need to explicitly set ./xmlchange DATM_CPLHIST_DOMAIN_FILE=null? From our earlier discussions, I thought this was going to be the out-of-the-box default? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1950 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlxE3ORmV55PHWNn-t8mADhnt1Zyg2oks5spArUgaJpZM4Pt_Lq> .

mvertens · 2017-10-10T22:14:43Z

@billsacks -. It is the default but it helps to have it explicitly set as well. I have also addressed your comment as to the documentation.

mvertens · 2017-10-10T22:15:15Z

@billsacks, @gold2718 - I have explicitly added the fixes in the PR.

billsacks · 2017-10-11T16:28:11Z

@mvertens I tried setting up a case (out of alpha07d with new cime) with:

./create_newcase --case tg_new_1011 --compset T1850G --res f09_g16 --run-unsupported
cd tg_new_1011
./case.setup
./preview_namelists

This gave (among other things):

   Calling /glade2/scratch2/sacks/cesm_code/cesm2_0_alpha07d_newCime/cime/src/components/data_comps/dlnd/cime_config/buildnml
   ....  Obtaining DLND model domain info from stream sno.cplhist
   ....    lonname  must be xc
   ....    latname  must be yc
   ....    areaname must be area
   ....    fracname must be frac

Do these "must be" messages indicate problems? If not, I find them confusing, because this wording suggests to me that something is wrong.

mvertens · 2017-10-11T16:40:04Z

These do not indicate problems. They need to let the user know that if you use a stream file to obtain the domain and you are picking the stream file - rather than just having the first stream file be picked - then you need to have the following naming convention. I think its important to point this out - but maybe another wording would be useful.

…

On Wed, Oct 11, 2017 at 10:28 AM, Bill Sacks ***@***.***> wrote: @mvertens <https://github.com/mvertens> I tried setting up a case (out of alpha07d with new cime) with: ./create_newcase --case tg_new_1011 --compset T1850G --res f09_g16 --run-unsupported cd tg_new_1011 ./case.setup ./preview_namelists This gave (among other things): Calling /glade2/scratch2/sacks/cesm_code/cesm2_0_alpha07d_newCime/cime/src/components/data_comps/dlnd/cime_config/buildnml .... Obtaining DLND model domain info from stream sno.cplhist .... lonname must be xc .... latname must be yc .... areaname must be area .... fracname must be frac Do these "must be" messages indicate problems? If not, I find them confusing, because this wording suggests to me that something is wrong. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1950 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlxE4eKU8yp3Nt_8eXm6VeT2IsWo-1Uks5srOydgaJpZM4Pt_Lq> .

I found these confusing (seeming to indicate a problem), and Mariana was okay with just removing them.

billsacks

This looks great. Thanks a lot for adding cplhist topo capability, and also thanks for cleaning up a bunch of inconsistent things with the data models and improving the documentation!

I have pushed some commits with minor cleanup that I was pretty confident about. I have added a few line comments with some things that I was less confident about and wanted to run by you.

billsacks · 2017-10-11T18:39:10Z

doc/source/data_models/data-lnd.rst

   "DLND_CPLHIST_DIR", "Coupler history data mode directory containing coupler history data"
   "DLND_CPLHIST_YR_ALIGN",  "Coupler history data model simulation year corresponding to data starting year"
   "DLND_CPLHIST_YR_START", "Coupler history data model starting year to loop data over"
   "DLND_CPLHIST_YR_END", "Coupler history data model ending year to loop data over"

+.. note:: If ``DLND_MODE`` is set to ``CPLHIST``, it is normally assumed that the model domain will be identical to **all** of the stream domains. To ensure this, the xml variables ``LND_DOMAIN_PATH`` and ``LND_DOMAIN_FILE`` are ignored and a valid setting **must be given** for ``DLND_CPLHIST_DOMAIN_FILE``. If ``DLND_CPLHIST_DOMAIN_FILE`` is set to ``null``, then the dlnd component domain information is read in from the first coupler history file in the target stream and  it is assumed that the first coupler stream file that is pointed to contains the domain  information for that stream. This is the default mode that should be used for this mode. Alternatively, ``DLND_CPLHIST_DOMAIN_FILE`` can be set to ``$LND_DOMAIN_PATH/$LND_DOMAIN_FILE`` in a non-default configuration.


Based on our email communication, I think this documentation for DLND_CPLHIST_DOMAIN_FILE is wrong - specifically related to the default behavior (it seems copied from DATM, but I thought the operation was different for DLND).

billsacks · 2017-10-11T19:01:26Z

src/components/data_comps/datm/cime_config/config_component.xml

@@ -17,7 +17,7 @@
    <desc option="CRUv7"> CLM CRU NCEP v7 data set </desc>
    <desc option="GSW"> GSWP3 data set </desc>
    <desc option="GSWP3v1"> GSWP3v1 data set </desc>
-    <desc option="CPLHIST"> Coupler hist data set </desc>
+    <desc option="CPLHIST"> Coupler hist data set (assumes that model grid and stream domains are the same)</desc>


Is this actually assumed, or is it just the default?

I think this is the default IF the domain file is set to null. In this case if you ask for a different model grid and you obtain the domain from the first stream file then you will have an inconsistent set of mapping files relative to the actual domain that is being used. In that case I think the model should abort - but I've never tried it.

Thanks for the clarification. But I guess my point is: I don't think CPLHIST mode itself assumes that the model grid and stream domains are the same. Rather, the default if the domain file is set to null assumes that these two domains are the same.

I'm not quite sure if I'm right in my understanding there... and if I am right, I'm not quite sure how and where to document this... but I can think more about how to document this clearly if you want me to.

Yes - I agree with your statement that the default if the domain file is set to null assumes that these two domains are the same. I think that one possible way to state this is that CPLHIST mode is strongly recommended to be run only when the model domain and the couple history forcing are on the same domain. Thoughts?

Yes, I like that wording.

billsacks · 2017-10-11T22:40:32Z

src/components/data_comps/datm/cime_config/config_component.xml

    </values>
    <group>run_component_datm</group>
    <file>env_run.xml</file>
-    <desc>Simulation year corresponding to starting year (only used for CPLHIST3HrWx mode)</desc>
+    <desc>Simulation year corresponding to starting year (only used when DATM_MODE is CPLHIST)</desc>


Since the user now needs to set this manually, I think the documentation should be expanded. Here's my suggestion, but I'd like you to confirm this, because I'm not confident of my understanding of this variable:

<desc>Simulation year corresponding to DATM_CPLHIST_YR_START. This will usually be the same as the year in RUN_STARTDATE, but it can be set to a different year to start the simulation with a different year of forcing data. (Only used when DATM_MODE is CPLHIST.)</desc>

Whatever documentation we add for this one, the same documentation should be added for DLND and DROF CPLHIST_YR_ALIGN.

I'm not sure this is right. Say you had an 1850 simulation and you were writing coupler history output for years 220-230. Now you wanted to use that output to spin-up the ocean. You now start from year 1 in the ocean - but cycle over years 220-230. I think it will be more confusing to add this documentation. Frankly, I think that cplhist mode is really geared to more expert users. We could give some examples in the documentation for clarity - but I think that would be better in the user's guide as opposed to the xml file.

My understanding of the align year is that, in order to do what you're suggesting, you would set the align year to 1, which I think is consistent with my suggested wording. And I do still think that, for spinup runs, the typical operation would be to set the align year to the year in RUN_STARTDATE... and if that is correct (we could ask Keith L & Keith O to confirm), I do feel it would be helpful to add a brief sentence to that effect so that people don't have to dig all through the user's guide to see how to set this unintuitive value.

But I'll let that go if you still disagree :-) - in which case, I'm fine with putting additional documentation in the user's guide instead of the xml file, as long as it appears somewhere. Your idea of providing some examples of typical use cases sounds like a good one, regardless of whether this extra documentation appears in the xm file. (And I'm fine with that being deferred to a separate issue/pr - in that case we should open an issue for it.)

billsacks · 2017-10-11T22:47:01Z

src/components/data_comps/datm/cime_config/namelist_definition_datm.xml

@@ -1616,8 +1628,6 @@
      scientific modes supported by the model, they are listed below.  The
      mode is selected by a character string set in the strdata namelist
      variable dataMode.
-      datamode = "CPLHIST"


I think CPLHIST should also be removed from the list of valid values for this variable; do you agree?

Yes! Good catch. I'm changing that. Along with some other small fixes I am seeing that need to be made.

billsacks · 2017-10-11T22:51:30Z

src/components/data_comps/dlnd/cime_config/config_component.xml

+    <valid_values></valid_values>
+    <default_value>null</default_value>
+    <values match="last">
+      <value compset="2000.*_DLND%SCPL" grid="l%0.9x1.25">$LND_DOMAIN_PATH/$LND_DOMAIN_FILE</value>


Why are there these duplicated entries for all the different time periods? Why not have a single entry with a compset match of "_DLND%SCPL"?

Another good catch! You're right. I'll change that.

billsacks · 2017-10-11T22:52:03Z

src/components/data_comps/dlnd/cime_config/config_component.xml

+      NOTE: if this is set to 'null', then domain information is read in from the first
+      coupler history file in the target stream and it is assumed that the first coupler
+      stream file that is pointed to contains the domain information for that stream.
+      This is the default mode that should be used when DLND_MODE is CPLHIST or GLC_CPLHIST.


This last sentence is wrong for DLND, I think.

Thanks for catching this as well. Actually this was wrong in DATM, DROF and DLND. I've changed the xml documentation in all three config_component.xml files.

billsacks · 2017-10-11T22:52:51Z

src/components/data_comps/dlnd/cime_config/config_component.xml

@@ -76,7 +98,7 @@

  <entry id="DLND_CPLHIST_YR_ALIGN">
    <type>integer</type>
-    <default_value>1</default_value>
+    <default_value>-999</default_value>


See above comment for DATM_CPLHIST_YR_ALIGN: whatever documentation is added for that one should also be added for DLND_CPLHIST_YR_ALIGN

mvertens · 2017-10-12T02:43:23Z

I would run this by Keith L and Keith O to determine if how they use this. If we can clarify this in one or two sentences then I am fine with bringing this into config_component.xml. I just would rather not have a long explanation.

…

On Wed, Oct 11, 2017 at 8:20 PM, Bill Sacks ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/data_comps/datm/cime_config/config_component.xml <#1950 (comment)>: > </values> <group>run_component_datm</group> <file>env_run.xml</file> - <desc>Simulation year corresponding to starting year (only used for CPLHIST3HrWx mode)</desc> + <desc>Simulation year corresponding to starting year (only used when DATM_MODE is CPLHIST)</desc> My understanding of the align year is that, in order to do what you're suggesting, you would set the align year to 1, which I think is consistent with my suggested wording. And I do still think that, for spinup runs, the typical operation would be to set the align year to the year in RUN_STARTDATE... and if that is correct (we could ask Keith L & Keith O to confirm), I do feel it would be helpful to add a brief sentence to that effect so that people don't have to dig all through the user's guide to see how to set this unintuitive value. But I'll let that go if you still disagree :-) - in which case, I'm fine with putting additional documentation in the user's guide instead of the xml file, as long as it appears somewhere. Your idea of providing some examples of typical use cases sounds like a good one, regardless of whether this extra documentation appears in the xm file. (And I'm fine with that being deferred to a separate issue/pr - in that case we should open an issue for it.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1950 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlxEzyAs9JK8nNCpQrB2lyc8Rfe6ZG5ks5srXd6gaJpZM4Pt_Lq> .

Description from Keith Lindsay, example from Keith Oleson

…1950) Limit performance archiving to specific projects on each system To allow for automatic performance archiving on ACME production platforms for ACME allocations without also forcing this on non-ACME allocations, add a new XML variable to config_machines.xml and env_run.xml: <SAVE_TIMING_DIR_PROJECTS>proj1,proj2</SAVE_TIMING_DIR_PROJECTS> If (a) PROJECT is one of the projects in this list and (b) SAVE_TIMING is true and (c) SAVE_TIMING_DIR is a legal location, then performance archiving into SAVE_TIMING_DIR will take place. Otherwise it will not. If the first element of the list is ANY, then any allocation will pass test (a). If SAVE_TIMING_DIR_PROJECTS is missing for a given machine in config_machines.xml, then no allocation will pass test (a). Fixes #1949 BFB * origin/worleyph/cime/save_timing_dir_projects: put SAVE_TIMING_DIR_PROJECTS in env_run.xml simplify implementation of is_save_timing_dir_project fix typo in comment in machines.py change logger.warning to logger.info modify logger output when archiving performance data Improve support for project-specific performance archiving

Mariana Vertenstein added 6 commits October 2, 2017 13:43

changes to get datm cplhist mode working with a topo stream

ea7b212

more robust implementation of CPLHIST mode and refactor of format out…

367f381

…put to be consistent with python3

updates to data model docs for cplhist mode

bd6e114

updates of data model documentation

c458573

bug fixes

84ea738

minor format fixes

0526b1e

mvertens added the in progress label Oct 4, 2017

mvertens requested review from billsacks and jedwards4b October 4, 2017 17:48

mvertens assigned billsacks Oct 4, 2017

mvertens changed the title ~~Mvertens/cplhist topo~~ refactor CPLHIST mode and add DATM CPLHIST topo capability Oct 4, 2017

gold2718 reviewed Oct 4, 2017

View reviewed changes

jedwards4b approved these changes Oct 5, 2017

View reviewed changes

billsacks added 4 commits October 11, 2017 14:54

Minor documentation edits

b06b267

Remove some log statements

bf2bb2b

I found these confusing (seeming to indicate a problem), and Mariana was okay with just removing them.

Make some documentation consistent

7e054db

fix whitespace

7dc983a

billsacks requested changes Oct 11, 2017

View reviewed changes

consistency changes for CPLHIST defaults and updated documentation

ade2ccf

billsacks mentioned this pull request Oct 12, 2017

CIME Users Guide Documentation #1519

Closed

billsacks added 3 commits October 13, 2017 14:22

Minor documentation fix from Mariana

6e4d431

Improve documentation of CPLHIST_YR_ALIGN variables, from Keith Lindsay

e879189

Add details on year_align

d02a0a4

Description from Keith Lindsay, example from Keith Oleson

billsacks approved these changes Oct 13, 2017

View reviewed changes

billsacks merged commit 16adad1 into master Oct 13, 2017

billsacks removed the in progress label Oct 13, 2017

mvertens deleted the mvertens/cplhist_topo branch October 18, 2017 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor CPLHIST mode and add DATM CPLHIST topo capability #1950

refactor CPLHIST mode and add DATM CPLHIST topo capability #1950

mvertens commented Oct 4, 2017 •

edited

Loading

gold2718 Oct 4, 2017

mvertens Oct 4, 2017

gold2718 commented Oct 4, 2017

billsacks commented Oct 4, 2017

billsacks commented Oct 4, 2017

billsacks commented Oct 4, 2017

mvertens commented Oct 4, 2017 via email •

edited

Loading

mvertens commented Oct 10, 2017

mvertens commented Oct 10, 2017

billsacks commented Oct 11, 2017

mvertens commented Oct 11, 2017 via email

billsacks left a comment

billsacks Oct 11, 2017

billsacks Oct 11, 2017

mvertens Oct 12, 2017

billsacks Oct 12, 2017

mvertens Oct 12, 2017

billsacks Oct 12, 2017

billsacks Oct 11, 2017

mvertens Oct 12, 2017

billsacks Oct 12, 2017

billsacks Oct 11, 2017

mvertens Oct 12, 2017 •

edited

Loading

billsacks Oct 11, 2017

mvertens Oct 12, 2017

billsacks Oct 11, 2017

mvertens Oct 12, 2017

billsacks Oct 11, 2017

mvertens commented Oct 12, 2017 via email

refactor CPLHIST mode and add DATM CPLHIST topo capability #1950

refactor CPLHIST mode and add DATM CPLHIST topo capability #1950

Conversation

mvertens commented Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gold2718 commented Oct 4, 2017

billsacks commented Oct 4, 2017

billsacks commented Oct 4, 2017

billsacks commented Oct 4, 2017

mvertens commented Oct 4, 2017 via email • edited Loading

mvertens commented Oct 10, 2017

mvertens commented Oct 10, 2017

billsacks commented Oct 11, 2017

mvertens commented Oct 11, 2017 via email

billsacks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvertens Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvertens commented Oct 12, 2017 via email

mvertens commented Oct 4, 2017 •

edited

Loading

mvertens commented Oct 4, 2017 via email •

edited

Loading

mvertens Oct 12, 2017 •

edited

Loading