Fixed search for fx files when no `mip` is given #1216

schlunma · 2021-07-06T19:45:54Z

This PR fixes the automatic retrieval of fx files when no mip is specified.

I agree with @sloosvel that a predefined order for the mip tables is not useful due to the many different project tables. Therefore I just implemented an alphabetic ordering, i.e., the first mip that has data is automatically selected. To bypass this, the user has to define a specific mip in the recipe. I added a paragraph to the documentation that describes this process.

This should go into 2.3.1 since this fixes a bug that (1) lets the test fails and (2) might lead to the tool wrongly complaining about missing data. #1169 only disabled the tests and didn't actually fix the bug.

Closes #1159.

Link to documentation: https://esmvaltool--1216.org.readthedocs.build/projects/ESMValCore/en/1216/recipe/preprocessor.html#fx-variables-as-cell-measures-or-ancillary-variables

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 · 2021-07-06T19:47:44Z

Codecov Report

Merging #1216 (8cd5dcf) into main (695fc48) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
+ Coverage   85.51%   85.53%   +0.01%     
==========================================
  Files         188      188              
  Lines        9136     9148      +12     
==========================================
+ Hits         7813     7825      +12     
  Misses       1323     1323

Impacted Files	Coverage Δ
esmvalcore/_recipe.py	`91.25% <100.00%> (+0.13%)`	⬆️

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 695fc48...8cd5dcf. Read the comment docs.

zklaus · 2021-07-07T09:04:47Z

The issue with this approach is that there are experiments, where there are two or more fx files for the same variable and it is impossible to determine a generic right order, e.g. here. I think #1159 (comment) is relevant. In other words, just specify the correct table in the recipe.

zklaus

I don't think any generic ordering can meaningfully resolve existing ambiguity in the table selection.

zklaus · 2021-07-07T09:15:33Z

I do like the documentation improvements. Let's work that into 2.4.0.

schlunma · 2021-07-07T09:19:11Z

I don't think any generic ordering can meaningfully resolve existing ambiguity in the table selection.

I agree, that's why I added this note to the documentation:

.. note::
   Some fx variables exist in more than one table (e.g., ``volcello`` exists in
   CMIP6's ``Ofx`` and ``Omon`` table or ``sftgif`` exists in CMIP6's ``fx``
   and ``IyrAnt`` table). If a specific table is desired, this needs to be
   specified by ``mip``, otherwise the table is selected based on the
   alphabetical order and data availability.

Do you want the tool to fail if there's is ambiguity? Or issue a warning?

I still thinks this should go into 2.3.1 since this merely fixes a bug that was introduced in 2.3.0.

zklaus · 2021-07-07T09:24:22Z

I think the tool should fail if there is ambiguity.

As for the bug, are you referring to #1159? If so, in my view, that doesn't reflect the introduction of a new bug, but the uncovering of an existing bug in the test, namely the ambiguity. The solution seems clear: specify the correct table in the recipe used in the test. I am not sure why @sloosvel's comment there went unanswered.

If you mean another bug, I am sorry I missed it. In that case, could you point me to the right issue, please?

schlunma · 2021-07-07T09:38:52Z

The tests were designed for the pre-#999 situation and did not fail there. This old implementation did only check tables that included fx in their name, so the new version is definitely an improvement here. However, the point is that the tests didn't fail on any machine before #999 and failed after it on some machines.

#1169 "solved" this by disabling the tests. I don't think that's the way to go. This PR fixes the tests by introducing a machine-independent ordering for the tables.

I agree with you that in case of ambiguity the tool should fail, but this would be a new feature and should go into v2.4 (I can open another PR for that).

zklaus · 2021-07-07T09:48:21Z

It seems we agree that the bug is in the test. We also agree that disabling the test is not really the right way to address this. But there seems to be a clear solution: fix the test. Why would we want to change the behavior of the tool to work around a buggy test?

schlunma · 2021-07-07T10:01:34Z

Added break statement: This was missing in Load fx variables as cube cell measures / ancillary variables #999 (see Fx files are selected differently depending on what OS the code runs on #1159 (comment) for further information.
Added sorted: This was not necessary pre-Load fx variables as cube cell measures / ancillary variables #999 but is necessary now to ensure a consistent behavior across machines. This can be removed once we make the tool fail in case of ambiguity.
Changed message in RecipeError: This only changes the message in case a mip is given. I can remove this if you like, but I think there is no harm in having this.

sloosvel · 2021-07-07T10:50:45Z

I think the changes make the documentation much better.

I agree that that break should not have been removed, it went over my head. And in case of ambiguity (which I have yet to see an exp with the same fx var in multiple mips, but who knows), specifying the mip solves the issue so it's much simpler than trying to come up with a search order that would fit all projects.

valeriupredoi · 2021-07-07T11:46:07Z

@sloosvel - @zklaus provides an example of exactly that - areacellg in two mips, here and there are others (I believe even volcello that could be fx or Ofx if it's time variant or not, Klaus correct me if I'm wrong pls). I like this approach but I think we need the run to stop if it encounters multiple mips for the same fx variable, and prompt the user to make up their mind and add valid mip. This saves us from any future headaches due to ESGF data standards variations. Note the current lazy approach to not produce these sort of variables for all exps and mips and shortshifting by making use of just a small data production eg fx vars are produced for one ensemble only or one experiment only

zklaus · 2021-07-07T11:54:34Z

(I believe even volcello that could be fx or Ofx if it's time variant or not, Klaus correct me if I'm wrong pls).

Almost right: volcello can be in one or more of Ofx, Omon, Oyr, and Odec.

schlunma · 2021-07-07T12:00:00Z

All right, I will alter the code so that it raises an error if multiple mips are encountered that include a given fx variable. I still believe that this should go into the release 2.3.1, since it's clearly a fix of a broken feature.

valeriupredoi · 2021-07-07T12:03:35Z

cheers @zklaus and cheers @schlunma too, and I too believe this should be part of 2.3.1 but Klaus is the release bossman 👍

zklaus · 2021-07-07T12:05:51Z

Alright, I am not against putting this in 2.3.1, but it still needs a bit of work (I spotted some things in the documentation at least and I think we don't have any formal review yet), so it might come down to a question of timing.

schlunma · 2021-07-07T14:56:35Z

Alright, now the tool raises an error if an fx variable is found in multiple tables. I also adapted the documentation accordingly.

valeriupredoi

excellent work, cheers @schlunma 🍺 One nag for me, I'd change that from a Note to a Warning 👍

doc/recipe/preprocessor.rst

Co-authored-by: Valeriu Predoi <[email protected]>

zklaus

Sorry, only made it just to the actual code; have to continue later. Anyway, I think the most important comment is the last one.

doc/recipe/preprocessor.rst

esmvalcore/_recipe.py

zklaus

Sorry if something is very weird; I had some trouble with the github web interface.

doc/recipe/preprocessor.rst

esmvalcore/_recipe.py

doc/recipe/preprocessor.rst

schlunma · 2021-07-08T14:38:50Z

I don't know why the tests are failing, I though these

TypeError: cannot compare cftime.DatetimeGregorian(300, 1, 16, 12, 0, 0, 0) and datetime.datetime(300, 1, 16, 12, 0) (different calendars)

errors were fixed by #1173?

valeriupredoi · 2021-07-08T14:50:18Z

pls merge main in here, if you done that since yesterday afternoon then the test container needs to rebuild with the new environment from #1197 (I belive it's done that last night) - give it a day, it'll do it, if not bugger @bouweandela see if it's stuck. Note that you can run the tests locally too and see it works

schlunma · 2021-07-08T15:47:55Z

After the merge of main the error got more interesting 😄

ValueError: Tag 'andela_bouwe' does not exist in section 'authors'

valeriupredoi · 2021-07-08T15:52:28Z

yes, @bouweandela is famous for causing SegFaults 🤣 It's a SegFault, Manu, am rerunning it just for you 😁

schlunma · 2021-07-09T10:30:46Z

The error didn't disappear after 3 restarts, so I guess there is something wrong here... Tests run fine locally.

valeriupredoi · 2021-07-09T10:48:33Z

hang on, lemme investigate 🔍

valeriupredoi · 2021-07-09T11:02:09Z

running tests locally I am getting this:

E       assert "Requested fx variable 'volcello' for dataset 'CanESM5' of project 'CMIP6' is available in more than one CMOR table for 'CMIP6': ['Ofx', 'Oyr', 'Odec', 'Omon']" in "Requested fx variable 'volcello' for dataset 'CanESM5' of project 'CMIP6' is available in more than one CMOR table for 'CMIP6': ['Oyr', 'Ofx', 'Odec', 'Omon']"
E        +  where "Requested fx variable 'volcello' for dataset 'CanESM5' of project 'CMIP6' is available in more than one CMOR table for 'CMIP6': ['Oyr', 'Ofx', 'Odec', 'Omon']" = <[RecursionError('maximum recursion depth exceeded while getting the repr of an object') raised in repr()] RecipeError object at 0x7fc19b08a4c0>.message

tests/integration/test_recipe.py:2799: AssertionError

and no SegFault after 2 runs, after updating to Python 3.9.6 too

schlunma · 2021-07-09T11:05:56Z

Oh, that's again related to the machine-dependent ordering. Lemme change that real quick.

valeriupredoi · 2021-07-09T11:12:34Z

you know what? That recursion depth error might have given me a massive clue why we get all those SegFaults anyway

valeriupredoi · 2021-07-09T11:13:17Z

Oh, that's again related to the machine-dependent ordering. Lemme change that real quick.

you not running Ubuntu too on your machine?

schlunma · 2021-07-09T11:26:21Z

Was doing the tests on mistral which runs on Red Hat Enterprise Linux

valeriupredoi · 2021-07-09T11:28:07Z

weird coz I think that Circle runs on CentOS - ah yes, not weird, I am silly, they're the same curry

schlunma · 2021-07-09T11:32:33Z

Thanks so much V., that seems to have done the trick! 🎉

@zklaus Any further comments from your side? I hope I addressed them all.

valeriupredoi · 2021-07-09T11:34:47Z

so funny - now the tests pass fine and no more SegFault - I am going to get to the bottom of those SegFaults so help me Lord Vader! 😁

schlunma · 2021-07-09T11:35:22Z

Haha, I hope you will!

zklaus · 2021-07-12T14:57:27Z

Nice work, @schlunma ! 🍻

* Fixed tests related to fx file retrieval * Raise error when fx variable is present in multiple tables * Update doc/recipe/preprocessor.rst Co-authored-by: Valeriu Predoi <[email protected]> * Updated documentation on fx variables * Used :ref: to link area_statistics and volume_statistics * Optimized fx doc * Only raise ambiguity error if fx files are found in mutiple tables * Fixed machine-dependent error-message Co-authored-by: Valeriu Predoi <[email protected]>

Fixed tests related to fx file retrieval

368b017

schlunma added the bug Something isn't working label Jul 6, 2021

schlunma added this to the v2.3.1 milestone Jul 6, 2021

schlunma requested review from zklaus, valeriupredoi and sloosvel July 6, 2021 19:45

schlunma self-assigned this Jul 6, 2021

zklaus requested changes Jul 7, 2021

View reviewed changes

zklaus mentioned this pull request Jul 7, 2021

Fx files are selected differently depending on what OS the code runs on #1159

Closed

zklaus modified the milestones: v2.3.1, v2.4.0 Jul 7, 2021

Raise error when fx variable is present in multiple tables

4cca1e5

schlunma modified the milestones: v2.4.0, v2.3.1 Jul 7, 2021

valeriupredoi approved these changes Jul 7, 2021

View reviewed changes

doc/recipe/preprocessor.rst Outdated Show resolved Hide resolved

Update doc/recipe/preprocessor.rst

a9b85da

Co-authored-by: Valeriu Predoi <[email protected]>

zklaus requested changes Jul 7, 2021

View reviewed changes

schlunma added 2 commits July 8, 2021 10:24

Updated documentation on fx variables

2e0d9dc

Used :ref: to link area_statistics and volume_statistics

cc60332

zklaus requested changes Jul 8, 2021

View reviewed changes

doc/recipe/preprocessor.rst Show resolved Hide resolved

esmvalcore/_recipe.py Show resolved Hide resolved

doc/recipe/preprocessor.rst Outdated Show resolved Hide resolved

schlunma added 2 commits July 8, 2021 15:29

Optimized fx doc

9dfd16d

Only raise ambiguity error if fx files are found in mutiple tables

ca039cf

Merge remote-tracking branch 'origin/main' into fix_fx_test

e49ddb5

Fixed machine-dependent error-message

8cd5dcf

zklaus approved these changes Jul 12, 2021

View reviewed changes

zklaus merged commit cde17af into main Jul 12, 2021

zklaus deleted the fix_fx_test branch July 12, 2021 14:57

valeriupredoi mentioned this pull request Sep 9, 2021

Optional flag that stops the run if any of data files not found #1282

Open

Fixed search for fx files when no mip is given #1216

Fixed search for fx files when no mip is given #1216

Conversation

schlunma commented Jul 6, 2021 • edited Loading

Before you get started

Checklist

codecov bot commented Jul 6, 2021 • edited Loading

Codecov Report

zklaus commented Jul 7, 2021

zklaus left a comment

Choose a reason for hiding this comment

zklaus commented Jul 7, 2021

schlunma commented Jul 7, 2021

zklaus commented Jul 7, 2021

schlunma commented Jul 7, 2021

zklaus commented Jul 7, 2021

schlunma commented Jul 7, 2021 • edited Loading

sloosvel commented Jul 7, 2021

valeriupredoi commented Jul 7, 2021

zklaus commented Jul 7, 2021

schlunma commented Jul 7, 2021

valeriupredoi commented Jul 7, 2021

zklaus commented Jul 7, 2021

schlunma commented Jul 7, 2021

valeriupredoi left a comment

Choose a reason for hiding this comment

zklaus left a comment

Choose a reason for hiding this comment

zklaus left a comment

Choose a reason for hiding this comment

schlunma commented Jul 8, 2021

valeriupredoi commented Jul 8, 2021

schlunma commented Jul 8, 2021

valeriupredoi commented Jul 8, 2021

schlunma commented Jul 9, 2021

valeriupredoi commented Jul 9, 2021 • edited Loading

valeriupredoi commented Jul 9, 2021

schlunma commented Jul 9, 2021

valeriupredoi commented Jul 9, 2021

valeriupredoi commented Jul 9, 2021

schlunma commented Jul 9, 2021

valeriupredoi commented Jul 9, 2021 • edited Loading

schlunma commented Jul 9, 2021

valeriupredoi commented Jul 9, 2021

schlunma commented Jul 9, 2021

zklaus commented Jul 12, 2021

Fixed search for fx files when no `mip` is given #1216

Fixed search for fx files when no `mip` is given #1216

schlunma commented Jul 6, 2021 •

edited

Loading

codecov bot commented Jul 6, 2021 •

edited

Loading

schlunma commented Jul 7, 2021 •

edited

Loading

valeriupredoi commented Jul 9, 2021 •

edited

Loading

valeriupredoi commented Jul 9, 2021 •

edited

Loading