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

cprnc: allow differences in field lists for time-constant fields #3051

Merged
merged 9 commits into from
Mar 28, 2019

Conversation

billsacks
Copy link
Member

@billsacks billsacks commented Mar 22, 2019

In cprnc: For files with an unlimited (time) dimension: Separately count
(1) missing time-varying fields and (2) missing time-constant
fields. Only (1) is considered in determining whether to report a final
difference in the field lists.

Before this, no distinction was made between time-varying
vs. time-constant fields in counting the number of missing
variables. (These counts were added in #2988). However, that led to
failures in some exact restart tests, because some time-constant fields
were on output files from one case but not the other (see #3007).

Here is sample output from cprnc for a few cases:

(1) Difference in the presence / absence of time-varying variables:

SUMMARY of cprnc:
 A total number of      6 fields were compared
          of which      0 had non-zero differences
               and      0 had differences in fill patterns
               and      0 had different dimension sizes
 A total number of      0 fields could not be analyzed
 A total number of      2 time-varying fields on file 1 were not found on file 2.
 A total number of      0 time-constant fields on file 1 were not found on file 2.
 A total number of      0 time-varying fields on file 2 were not found on file 1.
 A total number of      0 time-constant fields on file 2 were not found on file 1.
  diff_test: the two files DIFFER only in their field lists

(2) Difference in the presence / absence of time-constant variables, for
files that have a time dimension:

SUMMARY of cprnc:
 A total number of     13 fields were compared
          of which      0 had non-zero differences
               and      0 had differences in fill patterns
               and      0 had different dimension sizes
 A total number of      0 fields could not be analyzed
 A total number of      0 time-varying fields on file 1 were not found on file 2.
 A total number of      2 time-constant fields on file 1 were not found on file 2.
 A total number of      0 time-varying fields on file 2 were not found on file 1.
 A total number of      1 time-constant fields on file 2 were not found on file 1.
  diff_test: the two files seem to be IDENTICAL
      (But note that there were differences in field lists just for time-constant fields.)

(3) Difference in the presence / absence of time-constant variables, for
files that do NOT have a time dimension (note that this still results in
a DIFFER result; it seemed to me that that's what a user would want in
this case):

SUMMARY of cprnc:
 A total number of      5 fields were compared
          of which      0 had non-zero differences
               and      0 had differences in fill patterns
               and      0 had different dimension sizes
 A total number of      0 fields could not be analyzed
 A total number of      2 fields on file 1 were not found on file 2.
 A total number of      1 fields on file 2 were not found on file 1.
  diff_test: the two files DIFFER only in their field lists

The downside of this solution is that we wouldn't catch removals of
time-constant fields when doing baseline comparisons, and might overlook
this for interactive uses of cprnc. To address the issue of baseline
comparisons, I thought about adding a flag that treats time-constant
fieldlist diffs as differences
(#3007 (comment)), but
I have not done this here because this would have taken more time to
implement and added significantly more complexity (in both the cprnc
Fortran and in hist_utils.py) for uncertain benefit. (See the discussion in #3007
for more thoughts on this.)

Once this PR is approved and merged, I will update the cprnc builds on
hobart and cheyenne (currently they are using builds prior to #2988).

Test suite:

Test baseline: n/a
Test namelist changes: none
Test status: bit for bit

Fixes #3007

User interface changes?: Changes cprnc behavior slightly, as noted above

Update gh-pages html (Y/N)?: N

Code review: @jedwards4b @jgfouca @fischer-ncar

Also cc @rljacob @gold2718 @mnlevy1981 @ekluzek

These will test the upcoming behavior change (allowing field diffs in
time-constant variables in a file that has a time dimension).

(Note: the number of missing / extra fields is deliberately asymmetrical
because this will better test the new code.)
We're going to need this function in other contexts in an upcoming
commit. Other benefits:
- This keeps knowledge of the convention of -1 meaning no unlimited
  dimension out of compare_vars_mod
- This makes the code in compare_vars_mod more explicit in that it is
  looking at whether file 1 has an unlimited dimension
We'll need this in an upcoming commit
For files with an unlimited (time) dimension, provide two separate
counts:

(1) The number of missing fields with an unlimited (time) dimension

(2) The number of missing fields without an unlimited (time) dimension

Only (1) is considered in determining whether to report a final
difference in the field lists.

Resolves ESMCI#3007
@billsacks
Copy link
Member Author

scripts_regression_tests.py has completed on cheyenne. The only failures are ones that also failed in the baseline (cime5.7.9, on which this branch is based):

======================================================================
FAIL: test_appends_not_overriden (__main__.H_TestMakeMacros)
Test that machine-specific base value changes don't interfere with appends.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./scripts_regression_tests.py", line 3074, in test_appends_not_overriden
    tester.assert_variable_equals("FFLAGS", "-base2", env={"COMPILER": self.test_compiler})
  File "./scripts_regression_tests.py", line 2566, in assert_variable_equals
    self.parent.assertEqual(self.query_var(var_name, env, var), value)
AssertionError: '-base2 -debug1' != '-base2'

======================================================================
FAIL: test_appends_not_overriden (__main__.I_TestCMakeMacros)
Test that machine-specific base value changes don't interfere with appends.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./scripts_regression_tests.py", line 3074, in test_appends_not_overriden
    tester.assert_variable_equals("FFLAGS", "-base2", env={"COMPILER": self.test_compiler})
  File "./scripts_regression_tests.py", line 2661, in assert_variable_equals
    self.parent.assertEqual(self.query_var(var_name, env, var), value)
AssertionError: '-base2 -debug1' != '-base2'

----------------------------------------------------------------------
Ran 149 tests in 3281.197s

FAILED (failures=2, skipped=10)

Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

The tests you mention as failing both pass for me on master.

@fischer-ncar fischer-ncar removed their request for review March 28, 2019 19:52
@billsacks billsacks assigned jedwards4b and unassigned fischer-ncar Mar 28, 2019
@billsacks
Copy link
Member Author

@jedwards4b - @fischer-ncar removed himself as a reviewer. Given that you have already reviewed this, I have reassigned this to you.

@jedwards4b jedwards4b merged commit 95e117c into ESMCI:master Mar 28, 2019
@billsacks billsacks deleted the cprnc_allow_timeconst_fielddiffs branch April 2, 2019 16:02
jgfouca pushed a commit that referenced this pull request Aug 20, 2019
Improve accuracy of the SCM "Replay" mode

This PR improves the accuracy of the SCM Replay mode. Previous implementation
allowed the user to "replicate the behavior" of an E3SM run in SCM, but with
temperature errors on the order of ~0.1 K after a day. This PR will provide
the user with capability to produce "quasi-bit-for-bit" results with Replay mode,
with temperature errors on the order of ~1.e-5 K.

The reason why Replay mode is unable to provide fully B4B results is because
the dynamics tendency computed for Replay is not identical to the way that the
full model computes its dynamics tendencies (which involves sub-cycling in the
SE dynamical core). Thus, there is issue with roundoff. Note that in the past,
CAM Replay mode with Eulerian dy-core could produce "B4B" results but only
because if the flag to generate Replay mode output was turned on, they updated
their T and Q based on the tendency computed for Replay purposes. Thus the run
to generate Replay forcing was NOT b4b with a normal production run. We are
working on a truly B4B Replay mode for E3SM and a future PR will support this
which will involve higher precision computation of the dynamical tendencies.

This PR also renames the old "-camiop" flag to "-e3smreplay" flag to be more
current and consistent with E3SM. Other flags are renamed accordingly throughout the code.

Note that in this PR the e3sm_developer test "SMS_R_Ld5.ne4_ne4.FSCM5A97"
is an expected fail when comparing results to baseline. This is because
where the large-scale vertical velocity is read in and updated had to be
moved so that CLUBB could see it at the consistent timestep. It was validated
that results for selected tested cases (ARM97, GOAMAZON, DYCOMS-RF01, BOMEX)
have very minimal impacts with this fix included.

All other e3sm_developer tests pass.

[BFB] except for the SCM test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants