Skip to content
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

Checking for an active atmosphere before modifying CAM_CONFIG_OPTS (run_acme). #1586

Merged
merged 4 commits into from
Jun 28, 2017

Conversation

cameronsmith1
Copy link
Contributor

@cameronsmith1 cameronsmith1 commented Jun 15, 2017

A test has been added to run_acme.template.csh so that CAM_CONFIG_OPTS will only be modified if there is an active atmosphere model.

The problem is that we previously turned the COSP simulator on for watercycle simulations using

  $xmlchange_exe --id CAM_CONFIG_OPTS --append --val " -cosp"

However, this fails for compsets with data-atmospheres (because CAM_CONFIG_OPTS doesn't exist in env_build.xml .

I also added a dummy usr_nl_clm for the convenience of users.

We turn the COSP simulator on for watercycle simulations using

  $xmlchange_exe --id CAM_CONFIG_OPTS --append --val " -cosp"

However, this fails for compsets with data-atmospheres. Hence, a test
has been added to run_acme.template.csh to only turn COSP on when
CAM_CONFIG_OPTS is in env_build.xml
@cameronsmith1
Copy link
Contributor Author

cameronsmith1 commented Jun 15, 2017

Note: run_acme failed for an I-compset because of this issue. To make matters worse, the error message from xmlchange is misleading (see #1585).

This is a workaround for run_acme.

An alternative solution is to comment out the line, and rely on users to uncomment it when they want COSP. (pinging @golaz )

#NOTE: xmlchange has a bug which requires append to be specified with quotes and a leading space.
#NOTE: The xmlchange will fail if CAM is not active (eg a land only simulation), so we need to test that CAM_CONFIG_OPTS is in env_build.xml

grep CAM_CONFIG_OPTS env_build.xml >> /dev/null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

./xmlquery can also be used here instead of grep:
./xmlquery CAM_CONFIG_OPTS >> /dev/null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good idea. I'll test it out.

The test for CAM_CONFIG_OPTS stopped run_acme execution because
run_acme uses the 'csh -e' option, and the test returned a non-zero
status when it was missing.  A similiar test using xmlquery had the
same problem.  Hence, the test has been replaced with an xmlquery of
the value of COMP_ATM; if it is 'datm' then the data atmosphere is
being used.  This new test is clearer, and resolves the problem.

This commit also includes a template for modifying usr_nl_clm, for the
convenience of users.
@cameronsmith1
Copy link
Contributor Author

When implementing the suggestion of @singhbalwinder , I found another problem: the test for CAM_CONFIG_OPTS stopped run_acme execution because run_acme uses the 'csh -e' option, and the test returned a non-zero status when CAM_CONFIG_OPTS was missing. A similar test using xmlquery had the same problem. Hence, the test has been replaced with an xmlquery of the value of COMP_ATM; if it is 'datm' then the data atmosphere is being used. I think this new test is clearer, and it resolves the problem.

This commit also includes a template for modifying usr_nl_clm, for the convenience of users. It would be nice to have a separate PR, but I don't think it is worth the effort.

*********1*********2*********3*********4*********5*********6*********7**
Longer commit message body describing the commit.
Can contain lists as follows:
	* Item 1
	* Item 2
	* Item 3

A good commit message should be written like an email, a subject
followed by a blank line, followed by a more descriptive body.

Can also contain a tag at the bottom describing what type of commit this is.
[BFB] - Bit-For-Bit
[FCC] - Flag Climate Changing
[Non-BFB] - Non Bit-For-Bit
[CC] - Climate Changing
[NML] - Namelist Changing

See confluence for a more detailed description about these tags.
@rljacob
Copy link
Member

rljacob commented Jun 23, 2017

@cameronsmith1 please fix the title to something more formal and in verb noun format.

@cameronsmith1 cameronsmith1 changed the title To allow data-atm compsets to work, I added a test for CAM_CONFIG_OPTS. Checking for an active atmosphere before modifying CAM_CONFIG_OPTS in run_acme script. Jun 23, 2017
@cameronsmith1 cameronsmith1 changed the title Checking for an active atmosphere before modifying CAM_CONFIG_OPTS in run_acme script. Checking for an active atmosphere before modifying CAM_CONFIG_OPTS (run_acme). Jun 23, 2017
@cameronsmith1
Copy link
Contributor Author

@rljacob , I have modified the title and initial comment for this PR, in preparation for merging to next/master.

NOTE: There is another PR open for run_acm (#1594). The changes do not conflict, other than they currently both use the same run_acme number.

@mfdeakin-sandia
Copy link
Contributor

mfdeakin-sandia commented Jun 23, 2017

Sorry I must have missed this; is this ready for merging?

Copy link
Contributor

@mfdeakin-sandia mfdeakin-sandia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me

@cameronsmith1
Copy link
Contributor Author

It would be good to get concurrence from @PeterCaldwell. And there is the need to deconflict the version number with #1594 . Otherwise, I think this is ready to go.

There was an independent PR (#1594) which became version 3.0.9,
so changing the version for this PR to 3.0.10.
@cameronsmith1
Copy link
Contributor Author

cameronsmith1 commented Jun 26, 2017

Since PR #1594 has been merged to master, I have upped the run_acme version number for this PR to 3.0.10

@PeterCaldwell can you give this PR a quick check?

Copy link
Contributor

@PeterCaldwell PeterCaldwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Sorry for the slow response.

@mfdeakin-sandia mfdeakin-sandia merged commit 4fa4c0b into master Jun 28, 2017
@mfdeakin-sandia mfdeakin-sandia deleted the cameronsmith1/atm/run_acme_2017-06-14 branch June 28, 2017 22:26
jgfouca pushed a commit that referenced this pull request Jul 14, 2017
Update cime to support full X configuration

Tested with ERS.f19_g16.X.yellowstone_intel only

Test suite:
Test baseline:
Test namelist changes:
Test status: [bit for bit, roundoff, climate changing]

Fixes [CIME Github issue #] 1587

User interface changes?:

Code review:
jgfouca pushed a commit that referenced this pull request Jul 14, 2017
Fixed issue with GET_REFCASE not being set correctly that was introduced with PR#1561 and
an issue with X compset mapping files that was introduced in #1586

Test suite: scripts_regression_tests.py
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes

User interface changes?:

Code review:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants