standardize QC testing workflow, requirements, documentation

[eclare108213]

Define and document a standard workflow for QC testing, including instructions for what to do when the configuration being modified is not part of the default setup. There should be a control run without the modification with which to compare a second run using the modified code; both runs may need some manual changes. For modifications that do cause major changes in model output, before and after plots of ice thickness should be made and posted to the relevant PR.

#331 might be used as a test case for defining this workflow.

Also related: #166 #333

[phil-blain]

Also related: #345 to make the plots :) !

[mattdturner]

I thought, to some extent, that this was already done. Code Compliance Testing Procedure has some of the software requirements for the QC test, and End to End Testing Procedure is an "end-to-end" testing procedure. I can see that this might need to be expanded upon, though.

[eclare108213]

Yes, thanks. Let's review what's already there and clarify as needed.

[eclare108213]

The plots I was thinking of are maps, and #345 is for time series. We don't have scripts to make maps (yet).

[mattdturner]

The current QC script will plot a map of 2-stage test failures, if the 2-stage test does indeed fail (only if all necessary Python packages are available). Here is an example:

Note that this QC test was done with randomly perturbed data in order to force a failure.

Should I update the QC script to plot a map of the base and test ice thicknesses for cases where either the 2-stage test or skill test fail? Or would it be better to just have a separate script to handle creating maps of data?

[mattdturner]

I just realized that the map has the x-axis flipped... I'll have to create a PR to fix that

[phil-blain]

I noticed that in the documentation here https://cice-consortium-cice.readthedocs.io/en/master/user_guide/ug_testing.html#code-compliance-testing-procedure there is a paragraph that seems to be a leftover from an early version of the doc :

Implementation notes: 1) Provide a pass/fail on each of the confidence intervals, 2) Facilitate output of a bitmap for each test so that locations of failures can be identified.

Maybe we could take this opportunity to clean that up.

Also, in order to close the present issue, I think we need 2 things :

1. Add some details in the end-to-end testing procedure section of the documentation, for example (additions between brackets) :

   # Run a full regression test to verify bit-for-bit
   ...
   # Check the results
   
   cd testsuite.test0
   ./results.csh
   
   [# Note which tests failed and determine which namelist options are responsible for the failures]

   and then

   # Run the QC test
   
   # Create a QC baseline
   # From the baseline sandbox
   
   ./cice.setup -m onyx -e intel --test smoke -g gx1 -p 44x1 --testid qc_base -s qc,medium
   cd onyx_intel_smoke_gx1_44x1_medium_qc.qc_base
   [# modify ice_in to activate the namelist options that were determined above]
   ./cice.build
   ./cice.submit
   
   # Create the t-test testing data
   # From the update sandbox
   
   ./cice.setup -m onyx -e intel --test smoke -g gx1 -p 44x1 -testid qc_test -s qc,medium
   cd onyx_intel_smoke_gx1_44x1_medium_qc.qc_test
   [# modify ice_in to activate the namelist options that were determined above]
   ./cice.build
   ./cice.submit

2. Review how a person opening a PR can access the testing procedure information.
   Right now the PR template has a link to https://github.com/CICE-Consortium/About-Us/wiki/Resource-Index#information-for-developers, and from there you can access the outline of the testing procedure by following either
   Contributing to model development -> Contribute to the Consortium
   or
   Software Development Practices Guide -> [scroll down] -> Required testing

   The problems I see with the current state of things is :

   • the testing procedure is not directly linked from the PR template
   • the testing procedure is duplicated in the Wiki
   • neither pages from the wiki link to the "End-to-end testing procedure" section of the
     documentation, which is where the most detailed information is.
   • the PR template should mention that we want maps of the ice thickness for non BFB changes.

[eclare108213]

All good suggestions, thanks @phil-blain. I think it's a good idea to explicitly ask for a QC test, with maps provided in the PR when not BFB. Whether this is done in the PR template or if a link is provided to a wiki or readthedocs with specific info is open for discussion. I'm generally in favor of keeping the PR template simpler, with links to other info as needed. I agree with @phil-blain though that the current link isn't direct enough.

[dabail10]

Interesting. I just ran the QC test, but did not get plots. I need to check into this more to see what I am missing.

[apcraig]

@dabail10 The modifications that generate the plots by default came in at #355. My guess is that the Solar PR #323 has not been updated to the master recently enough to have this feature.

[apcraig]

I believe this issue has been addressed and will close it. The documentation has been update thru a number of PRs and I think things are working OK and documentation is consistent. We recently added a few sentences to the qc documentation for further clarification.

QC testing should be carried out using configurations (ie. namelist settings) that exercise the active code modifications. Multiple configurations may need to be tested in some cases. Developers can contact the Consortium for guidance or if there are questions.