Implement three levels of CMOR checks strictness

[valeriupredoi]

Before you start, please read CONTRIBUTING.md.

Tasks

• Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
• This pull request has a descriptive title that can be used in a changelog
• Add unit tests
• Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
• If writing a new/modified preprocessor function, please update the documentation
• Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
• Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
• Please use yamllint to check that your YAML files do not contain mistakes
• If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.

---

Closes #338

[valeriupredoi]

@sloosvel and @jvegasbsc pls have a look at what I haz so far - I introduced the STRICT CMOR error (report_strict_error()) handled in a very particular way by report_errors() func - the logic behind this that we want, no matter what, to have the issues reported by these types of errors logged as errors to the very least, if not raised. Whereas regular errors can be not logged, can be logged but not raised or can be raised. Also bear in mind the --dry-run option that controls only the raise_exception flag (and nothing else). If you agree with this type of functionality, all's left is to replace report_error with report_strict_error for the cases in Table #338 (comment) with the edit from #338 (comment) 🍺

[sloosvel]

Thanks @valeriupredoi for starting the work. I am going to test it to fully understand what is being done.

[sloosvel]

I think that the messages are a bit confusing, the distinction between regular and strict errors is something that you have come up with according to your own judgement, not something that is given by cmor. I would keep the messages as they are now, with the only changes being the way they will get reported (warning / error) according to the flag. I can try it for instance with the standard_name to see better how this will work.

[valeriupredoi]

hi @sloosvel sorry for the late reply, I was out yesterday. Correct, the messages are my interpretation, please feel free to change as you think it's fit 🍺 Also, please take it from here with the changes for the different levels in #338 - as it's now all's let to be done is to replace report_error with report_strict_error for where's needed. I will correct for the unit tests when that's done 🍺

[valeriupredoi]

regular tests now pass, current fail due to py3.8/numba, PR #376

[bouweandela]

Thanks for adressing all the comments! @mattiarighi Could you please test? I have looked through most of the code twice, but it's a lot of changes.

[mattiarighi]

What would be a good strategy for testing this?
Running a few representative recipes with different level of cmor strictness?

[valeriupredoi]

wow boy am glad to see this ready for testing - awesome work guys! I recommend using a few datasets that we don't have fixes for yet and would normally fail a number of checks and running with different strictness levels. I can think of the HIRES mip's data that is, by all means, okay from a CF point of view but quite down with the sickness from a CMOR standard. I can post a recipe here in a few minutes

[valeriupredoi]

eg @mattiarighi do you have access to these models on DKRZ (I ran them on Jasmin but not sure if you have them on DKRZ too):

  # LOWRES models in /gws/nopw/j04/primavera5/stream1/CMIP6/HighResMIP
  - {dataset: CMCC-CM2-HR4, exp: hist-1950, grid: gn}  # CMCC
  - {dataset: CNRM-CM6-1, exp: hist-1950, ensemble: r1i1p1f2} # CNRM-CERFACS
  #- {dataset: EC-Earth3P, exp: hist-1950, ensemble: r1i1p2f1} # EC-Earth-Consortium
  - {dataset: ECMWF-IFS-LR, exp: hist-1950, ensemble: r1i1p1f1} # ECMWF
  - {dataset: HadGEM3-GC31-LL, exp: hist-1950, ensemble: r1i1p1f1, grid: gn} # MOHC
  - {dataset: MPI-ESM1-2-HR, exp: hist-1950, ensemble: r1i1p1f1, grid: gn} # MPI-M

  # HIRES models in /gws/nopw/j04/primavera5/stream1/CMIP6/HighResMIP
  - {dataset: CMCC-CM2-VHR4, exp: hist-1950, grid: gn}  # CMCC
  - {dataset: CNRM-CM6-1-HR, exp: hist-1950, ensemble: r1i1p1f2} # CNRM-CERFACS
  #- {dataset: EC-Earth3P-HR, exp: hist-1950, ensemble: r1i1p2f1} # EC-Earth-Consortium issues with lat-lon coords
  - {dataset: ECMWF-IFS-HR, exp: hist-1950, ensemble: r1i1p1f1} # ECMWF
  - {dataset: HadGEM3-GC31-HM, exp: hist-1950, ensemble: r1i1p1f1, grid: gn} # MOHC
  - {dataset: MPI-ESM1-2-XR, exp: hist-1950, ensemble: r1i1p1f1, grid: gn} # MPI-M

[mattiarighi]

I tested using this issue as a test case:

• with strict it stops with what usually is a warning (rlut: attribute positive not present), so it's correct
• with default it stops because of the above issue, also correct
• with relaxed same as default, I guess also correct?
• with ignore it still stops.

I'm not sure the above issue is the right test case, though.

[jvegreg]

I'm not sure the above issue is the right test case, though.

It is not, because it is an iris issue that is making us fail in an automatic fix

[mattiarighi]

Ok, I'll look for a different test case.

[valeriupredoi]

lemme test with the HIRES/LOWRES Primavera models

[valeriupredoi]

OK for CNRM-CM6-1 in HiRes mips (LowRes here):

• strict:

esmvalcore.cmor.check.CMORCheckError: There were errors in variable tas:
Added guessed bounds to coordinate lon from var lon
 Added guessed bounds to coordinate lat from var lat
in cube:
air_temperature / (K)               (time: 120; latitude: 128; longitude: 256)
     Dimension coordinates:

• default:
  PASS but report warning:

2020-03-10 16:15:58,179 UTC [121341] WARNING There were warnings in variable tas:
Added guessed bounds to coordinate lon from var lon
 Added guessed bounds to coordinate lat from var lat

• relaxed:
  PASS but report warning:

2020-03-10 16:19:09,868 UTC [124370] WARNING There were warnings in variable tas:
Added guessed bounds to coordinate lon from var lon
 Added guessed bounds to coordinate lat from var lat

• ignore:
  PASS but report warning:

2020-03-10 16:20:44,096 UTC [125197] WARNING There were warnings in variable tas:
Added guessed bounds to coordinate lon from var lon
 Added guessed bounds to coordinate lat from var lat

• no flag:
  identical to default

[valeriupredoi]

Unfortunately I can not reproduce any other data issues with these types because the HIRES Primavera guys did their job and corrected/updated the data in Dec-Jan, well done to them 🍺

[jvegreg]

HIRES Primavera guys did their job and corrected/updated the data in Dec-Jan, well done to them

PRIMAVERA people is very professional and competente. Don't you agree, @sloosvel? 😄

[valeriupredoi]

Handling of a regular data issue we see from #537

• strict:
  FAIL with

esmvalcore.cmor.check.CMORCheckError: There were errors in variable tos:
tos: does not match coordinate rank
in cube:
sea_surface_temperature / (degC)    (time: 1980; latitude: 232; longitude: 360)

• default:
  same as strict
• relaxed:
  PASS with warning:

[21341] WARNING There were warnings in variable tos:
tos: does not match coordinate rank

• ignore:
  same as relaxed
• no flag:
  same as strict

Guys, am happy with the tests - if @mattiarighi is OK I reckon it's safe to merge - and well done, this is a very useful bit of shtuff! 🍺

[debe-kevin]

Really nice stuff !