Apply clip_timerange to time dependent fx variables

[sloosvel]

Description

This PR makes sure to clip the timerange in time dependent fx variables

Closes #1602

Link to documentation:

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.

• 🧪 The new functionality is relevant and scientifically sound
• 🛠 This pull request has a descriptive title and labels
• 🛠 Code is written according to the code quality guidelines
• 🧪 and 🛠 Documentation is available
• 🛠 Unit tests have been added
• 🛠 Changes are backward compatible
• 🛠 Any changed dependencies have been added or removed correctly
• 🛠 The list of authors is up to date
• 🛠 All checks below this pull request were successful

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[codecov]

Codecov Report

Merging #1603 (c837987) into main (1d94671) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1603   +/-   ##
=======================================
  Coverage   91.38%   91.39%           
=======================================
  Files         204      204           
  Lines       11173    11176    +3     
=======================================
+ Hits        10211    10214    +3     
  Misses        962      962           

Impacted Files	Coverage Δ
esmvalcore/preprocessor/_ancillary_vars.py	98.57% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d94671...c837987. Read the comment docs.

[zklaus]

We really have an unhealthy confusion in terminology. Outside of ESMValTool, fx stands for fixed in time. It makes no sense to talk of time-dependent fx files or variables. In ESMValTool we are conflating a few things, three, I think. Those are time-independent things associated with fx, cell measures that give information about grid cells, and ancillary variables that give extra information related to other variables, for example, a quality flag or a threshold.

We should disambiguate our catch-all use of "fx" to better reflect what we are actually talking about. That will make fixes clearer and in the future help us to avoid this kind of situation.

I know that this is a long-standing issue that has not originated with this PR, but I am a bit concerned that rather than making progress in the right direction, we are further entrenching the issue with more and more code that eventually should be changed.

Would it be possible to try to reflect better terminology in some of the variable and function names?

[sloosvel]

Would calling cell measures and ancillary vars aux_variables in a general way work for you?

In any case, even if the code in the core is updated, the recipes would still to have to call them fx_variables. Otherwise they would break.

[zklaus]

Hm. I am not sure it's a good idea to mix those categories. Anyways, as far as I can see in this PR, we are really only dealing with cell measures, no? Perhaps then it is enough to refer to that, particularly in the test code.

[sloosvel]

I think it's convenient to have a general name to refer both ancillary variables and measure, because the steps of adding, checking and removing them from the cube are the same. In any case, even if the code changes, they are still going to be referred as fx_variables in the recipe. Maybe it would be better to leave it as it is for now and try to see if there is a better way to handle them in the upcoming months. As you said, this mix-up was not introduced in this PR.

[zklaus]

I left a few comments to illustrate what I had in mind, i.e. not to change all the code that you basically didn't touch here, but rather to slightly nudge things in the right direction by using proper terminology in new tests etc.

Tangentially worth noting #1082 (comment) and the associated branch main...split-recipe

[sloosvel]

Tangentially worth noting #1082 (comment) and the associated branch main...split-recipe

I guess this can be closed then?

[zklaus]

I'm not sure. @bouweandela, what do you think?

[bouweandela]

I'll try to get #1609 done before the deadline, but it's a pretty big reshuffle of the recipe parsing code (I'm trying to uncook the pot of spaghetti that _recipe.py has become and put all the strands straight and back in the package). So I'm not sure if I'll make it in time and it also depends on if we think it's a good idea to include a pretty big reshuffle of the recipe parsing code just before the feature freeze.. but well, there is a month full of testing after the feature freeze of course. So it depends a bit on how keen you are on getting this into v2.6.

[zklaus]

Alright, then I suggest we forget all my comments, just get this in now as a stop-gap measure and hope that your reshuffle improves the overall situation either for 2.6.0 or shortly thereafter.

[sloosvel]

I personally don't like working on big changes in a hurry, so if you cannot make it to 2.6 and need more time it's fine for me. There isn't any deliverable depending on that work anyway.

Besides, #1609 would also affect #1562 and I guess it should be merged before that and #1603.

[bouweandela]

It looks like I won't be able to get #1609 done in time, so please go ahead and include this in v2.6 if you need it in that release.

[sloosvel]

No worries! It would be good to test this though, as I tend to overlook use cases.

[schlunma]

I will have a look on this on Friday or early next week.

[schlunma]

Code looks good!

Successfully tested this with the following recipe (took forever to run though, I'm really looking forward to #1545):

documentation:
  description: Test

  title: Test

  authors:
    - schlund_manuel


preprocessors:

  area_stat:
    area_statistics:
      operator: mean
      fx_variables:
        areacella:

  vol_stat:
    volume_statistics:
      operator: mean
      fx_variables:
        volcello:
          mip: Omon


diagnostics:

  test:

    variables:
      tas:
        preprocessor: area_stat
        mip: Amon
        exp: historical
        start_year: 2000
        end_year: 2014
      thetao:
        preprocessor: vol_stat
        mip: Omon
        exp: historical
        timerange: '200001/200001'
      additional_datasets:
        - {dataset: ACCESS-ESM1-5, project: CMIP6, ensemble: r1i1p1f1, grid: gn}
    scripts:
      null

In the current main, this recipe fails. Thanks @sloosvel!!