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 does not FAIL if fields are missing #870

Closed
billsacks opened this issue Nov 30, 2016 · 6 comments
Closed

cprnc does not FAIL if fields are missing #870

billsacks opened this issue Nov 30, 2016 · 6 comments

Comments

@billsacks
Copy link
Member

billsacks commented Nov 30, 2016

From @gold2718 on August 28, 2015 18:29

While cprnc notes if fields cannot be compared, it does not FAIL a comparison if a field (NetCDF variable) goes missing. I can see allowing a new field but if we delete a field, should that count as an answer change? If not cprnc itself, then should a comparison test FAIL in this case?

Copied from original issue: CESM-Development#144

@billsacks
Copy link
Member Author

From @quantheory on August 28, 2015 18:49

I think that adding a variable and deleting a variable should both be considered FAIL. Adding a field can also be a mistake, albeit one that's usually less bad than accidentally removing a field.

Plus, if only deleting a variable was considered to FAIL, we would lose symmetry. It would be counter-intuitive if tag A failed baseline tests against tag B, but tag B passed baseline tests against tag A. Or to put it differently, baseline comparison should define an equivalence relation between tags.

@billsacks
Copy link
Member Author

Feeling is that this should be reported, but maybe labeled differently from a FAIL?

@billsacks
Copy link
Member Author

Decision in CSEG meeting: We'll have cprnc report a warning if fields have been either added or removed.

@billsacks billsacks self-assigned this Nov 30, 2016
@billsacks
Copy link
Member Author

billsacks commented Nov 30, 2016

I have addressed the cprnc side of this issue in CESM-Development#442

After that change comes to master, we'll still need to do the following:

  1. Make changes to the system test scripts to parse the new output
  2. Update and rebuild cprnc on all test platforms. Note that many of these likely still point to the old svn repository; for these, we'll need to add a clone of cime, and update the cprnc path in config_machines.xml to point to the new location.
  3. Run some tests that trigger the new behavior; make sure these are treated properly by the test system

Adding @fischer-ncar to the cc list

@billsacks
Copy link
Member Author

From @mnlevy1981 on May 9, 2016 19:15

Note that many of these likely still point to the old svn repository; for these, we'll need to add a clone of cime, and update the cprnc path in config_machines.xml to point to the new location.

I think we should do the following:

  1. Rename the existing cprnc checkout as cprnc.old (or delete it altogether)
  2. svn co https://github.com/CESM-Development/cime/trunk/tools/cprnc

No need to clone the entire CIME repo just to access cprnc when github gives us SVN hooks

@billsacks
Copy link
Member Author

@gold2718 and @mnlevy1981 - you two requested this a while ago. I made some changes to cprnc to support this a while ago here CESM-Development#442 . We need to determine what to do with this: First off, we need to determine how, if at all, the test scripts need to be changed to use this new output. Second, we need to determine whether it's worth bringing an updated version of CESM-Development#442 to master (i.e., updating the fortran cprnc), or if we should just wait for the python version, which is nearing completion, although wrapping it up isn't a super-high priority at the moment. Note that updates to the fortran would require recompiling on all test machines for them to actually take effect.

I'll keep the above-referenced PR open until we decide what to do.

Also cc @jgfouca @jedwards4b and @mfdeakin-sandia . @mfdeakin-sandia : note that, in addition to changing behavior somewhat, the above-referenced PR adds some tests for cprnc that we should make sure are working right in the python version.

@rljacob rljacob added the ready label Dec 9, 2016
rljacob pushed a commit that referenced this issue Feb 8, 2017
…C-02' (PR #870)

This PR modifies FC5AV1C-L so that it points to the FC5AV1C-02.
This will also change the WCYCL and CRYO cases to use FC5AV1C-02.

[CC]
@rljacob rljacob added Assigned and removed ready labels May 24, 2017
jedwards4b added a commit that referenced this issue Feb 11, 2019
Report missing variables in cprnc
This PR changes cprnc and the test system so that, if variables are
missing from one of the files (the baseline file or the new file), this
will be reported as a baseline failure, like this:

```
FAIL SMS_D_Ld3.f10_f10_musgs.I1850Clm50BgcCrop.cheyenne_intel.clm-default BASELINE ctsm1.0.dev025: FIELDLIST field lists differ
```
For cprnc itself, this changes the final status of cprnc so that, if
variables are missing from one of the files, the final status is not
"IDENTICAL": this final status will now look like:

```
diff_test: the two files DIFFER only in their field lists
```
In addition, the number of variables present in file2 but not file1 is
now reported in the final counts.

If there are both missing variables and differences in common variables,
cprnc's final result is the same as before - reporting a DIFF in this
case. i.e., this new result is only reported as the final status if all
common variables are identical: a real DIFF takes precedence over a
field list difference.

This PR also changes the python scripts so that they respond
appropriately to this new possible cprnc result: During baseline
comparisons, if the only differences are in field lists (no differences
in the actual values of variables), then the baseline phase will still
report a FAIL, but a comment will be added with a keyword, FIELDLIST and
a bit of description (similarly to what we do for BFAILs), facilitating
filtering out these FIELDLIST differences if they were expected. As with
the behavior of cprnc itself, FIELDLIST is only reported if the _only_
differences are in field lists: if there are any real differences, then
the output is the same as before (so that you won't miss any real
differences if you do something like `grep -v FIELDLIST`). (If there are
both FIELDLIST and BFAIL results for a given test, but no actual
differences in values, then we report MULTIPLE, so that neither `grep -v FIELDLIST` nor `grep -v BFAIL` would accidentally exclude the result.)

Test suite:

* `scripts_regression_tests` on cheyenne with both an old and new build
  of cprnc (ran on [63377a8](63377a8))
* manual tests of a test with baseline comparison with fieldlist diffs,
  and a test with both fieldlist and real diffs: checked TestStatus.log
  and TestStatus in these cases
* tests in tools/cprnc (via run_tests in that directory, with baseline
  comparisons against master version): ensured that the only diffs are
  as expected, and examined output of the new tests there (side-note
  about the new attribute-comparison test: attribute comparisons are
  currently asymmetrical: cprnc currently doesn't detect attributes
  present on file2 not on file1)

Test baseline: For cprnc's run_tests: [5fa1832](5fa1832)
Test namelist changes: none
Test status: bit for bit

Fixes #870

User interface changes?: none

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

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

No branches or pull requests

2 participants