Add basic support for variable mappings

[zklaus]

Description

Closes #1157

Link to documentation: https://esmvaltool--1124.org.readthedocs.build/projects/ESMValCore/en/1124/develop/extra_facets.html#

---

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

[zklaus]

I think this is functionally ready. It is still missing documentation and tests, plus the checks fail.

On the failing checks, I think all of them are spurious. In detail:

Codacy complains about two things, namely that _get_variable_details_for_project is too long a name, and that I disable one error locally. The local disabling is the right thing to do because it is complaining about a missing import precisely where I am testing for the right python version to allow the import.

The commit checks fail as follows:

• conda-build exceeds time limit
• documentation: success
• test: The tests succeed, but the uploading of the coverage fails due to missing CODACY_PROJECT_TOKEN, presumably because this branch lives in my fork.
• install and develop: these fail due to what looks like a typo in the test for a cmip5 fix:

>      self.assertEqual(dates[0].strftime('%Y%m%d%H%M'), ' 30001161200')
E       AssertionError: '30001161200' != ' 30001161200'

Questions:
@bouweandela, are you happy with the new naming?
@senesis, I think I addressed all your comments. Sorry for not responding directly to them. Do you think this is ok now?

[bouweandela]

install and develop: these fail due to what looks like a typo in the test for a cmip5 fix:

This looks like #876. I think the test could be improved so it uses the actual date instead of a string formatted version.

[bouweandela]

Looks good @zklaus!

[senesis]

@senesis, I think I addressed all your comments. Sorry for not responding directly to them. Do you think this is ok now?

I inserted one comment as a review (see above).

I also suggested (maybe in your private repository's branch) to make a change in cmor/_fixes/fix.py, to write

    def get_cube_from_list(self, cubes, short_name=None, mapping_key=None):
    ....
        if short_name is None:
            short_name = self.vardef.short_name
        short_name=self.var_mapping.get(mapping_key,short_name)

in order to add a capacity to this function (with backward compatibility), which allows fix method to easily access cubes using project's specific variable name (by providing project's specific key for this variable name)

[senesis]

That's look fine, except for the review comment below and maybe an extended version of get_cube_from_list (see other comment)

I was able to run a basic test with a recipe using simultaneously IPSL data in its two formats (mono-var and multi-var files) by defining two projects for that. For the time being, I cannot use project native6 for such a simultaneous use of two data formats (as long as the DRS cannot be chosen on a dataset instance basis)

[zklaus]

@stefsmeets, to make the tests work, I had to add validation for a new option in config_user.yml to the experimental config stuff. Apart from the simple addition, which permits essentially a single path or a list of paths, I needed to move from list to tuple in order to make the result hashable, which I need since it is used in an lru_cached function as an argument. So I added a _tuplify_validator alongside the _listify_validator. Could you please have a look at that and let me know what you think? Perhaps it's generally a good idea to move to tuples for these options?

[zklaus]

@senesis, I have added your suggestion for a mapping_key to the get_cube_from_list method. I have also implemented a config option for the extra_facets_dir, see discussion above.

Do you have any other comments?

[senesis]

Do you have any other comments?

@zklaus No , that's fine, thanks. I would like to update PR #1143 for this changes, so let me know when you're fully done, and also if you know the best way for updating my PR : locally rebasing my branch on yours and then pushing ?

[stefsmeets]

So I added a _tuplify_validator alongside the _listify_validator. Could you please have a look at that and let me know what you think?

Perhaps it is not necessary to copy the entire function, see my comment for a possible different approach! Can you also add some tests for the new function?

Perhaps it's generally a good idea to move to tuples for these options?

That depends on what is needed from the config. yaml arrays are loaded as lists in python. If they are needed as tuples in the code, then that could be changed in this part of the code using your validator.

[jvegreg]

Just a concept question. ¿Why you decided to keep the extra_facets separated instead of updating the var_info with them if not present? That's the way we do with things like institutes and activities for CMIP and has the advantage keeping most of the functions completely agnostic of the change

[jvegreg]

Just a concept question. ¿Why you decided to keep the extra_facets separated instead of updating the var_info with them if not present? That's the way we do with things like institutes and activities for CMIP and has the advantage keeping most of the functions completely agnostic of the change

I just see that this is indeed what you are doing, but then why have the extra parameter for the fix function?

[zklaus]

@jvegasbsc, thanks for looking at this. I'm sorry, but I am not sure I understand. I see no var_info around in the code that we are talking about, but let me ask it this way around: If we don't pass extra_facets, how does a fix get access to this information in, say, fix_metadata?

[bouweandela]

Quickly scanned the PR now, just two more comments on the documentation. I noticed that several of the tests are failing, but that looks unrelated to this pull requests. Trying to fix that in the fix-tests branch now..

[bouweandela]

Ideally all tests would need to be green before merging this.

[zklaus]

Assuming the fixes coming in from main took care of the circleci stuff (as yet to be seen), this leaves us with a couple of codacy issues. They are:

• locally disabling no-name-in-module. This should be ignored because this disabling is inside a guard that distinguished between the built-in module that's available from Python 3.9 onwards and the otherwise installed version.
• the suggestion to re-raise using from. I suggest to ignore this as well since raise ... from is a new Python feature and this is in a part of the code in which this PR only improves the formatting.
• A small number of formatting complaints in docstrings, that come about because of conflicting formatting standards in local and ci configuration (I think). Can also be ignored imho.