Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle IPSL-CM6 #1153

Merged
merged 45 commits into from
Jun 11, 2021
Merged

Handle IPSL-CM6 #1153

merged 45 commits into from
Jun 11, 2021

Conversation

senesis
Copy link
Contributor

@senesis senesis commented May 31, 2021

Description

Related to issue #1143. Allow to handle IPSL-CM6 data formats and file naming conventions.

Composed of :

The feature won't actually work without #1124 , but the proposed PR is compatible with the main branch before #1124 merge, in order to allow for immediate merge, and so to avoid bottlenecks. #1124 allows for finding the right file and the right variable in file when using ISPL-CM6 data.

Some documentation on the handling of native models has been introduced.

Performance in loading variables from IPSL's multi-variable datafiles with Iris is poor, even with the Iris 3.0.2 fix on that subject. This is why pre-selecting using CDO in a fix_data method is implemented. However, because CDO licence (GPL v2) is not compatible with ESMValTool licence policy, this feature is not activated by default, and no explicit dependency is introduced. The feature can be activated through a variable mappings file (see #1124), at those sites which accept to be dependent on CDO.


Before you get started

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.

Klaus Zimmermann and others added 7 commits May 12, 2021 11:38
* First attempte

* Do not require start and end years, add them later

* Correct condition

* Avoid key error in fx variables

* Consider two possible paths

* Fix function name

* Fix variable name

* Avoid duplicates in filename

* Add test for startdate expansion

* Add test for the replace tags method

* Rename tag

* Add documentation

* Allow to load subexps per timerange or as a whole

* Fix condition

* Remove 'all_years' functionality

* Fix conditions

* Fix flake

* Remove whitespace

Co-authored-by: Javier Vegas-Regidor <javier.vegas@bsc.es>
…grid (#507)

Co-authored-by: Bouwe Andela <b.andela@esciencecenter.nl>
Co-authored-by: Stef Smeets <s.smeets@esciencecenter.nl>
* sos and siconc fixed

* tests added

* test fixed

* fix flake8

* fix flake8

* fix codacy issue
* pin cf-units

* pin cf-units

* fix test

* fix test
@senesis senesis added the enhancement New feature or request label May 31, 2021
@senesis senesis added this to the v2.3.0 milestone May 31, 2021
@senesis senesis requested a review from jvegreg as a code owner May 31, 2021 09:57
@senesis senesis self-assigned this May 31, 2021
@senesis senesis requested review from bouweandela and zklaus May 31, 2021 09:59
Copy link
Contributor

@jvegreg jvegreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question: how can I access to a data sample so I can properly test it?

esmvalcore/cmor/_fixes/native6/ipsl_cm6.py Outdated Show resolved Hide resolved
esmvalcore/cmor/_fixes/native6/ipsl_cm6.py Outdated Show resolved Hide resolved
@senesis
Copy link
Contributor Author

senesis commented May 31, 2021

One question: how can I access to a data sample so I can properly test it?

An example of multi-variable file for IPSL-CM6 is available here (on my Google Drive)
Data is not enough : this PR anticipates on a merge of #1124, I tested the whole on a local branch which included #1124. I can push such a branch to the repository if you would like to run a test

@senesis
Copy link
Contributor Author

senesis commented Jun 1, 2021

@alistairsellar : you may wish to know that this PR exists

@zklaus
Copy link

zklaus commented Jun 2, 2021

Quick side note on the slow loading: I am just talking to some iris developers and it seems they are hitting the exact same problem, also while dealing with an XIOS generated multi-variable output file for the next model generation. There doesn't seem to be an issue about it yet, but I forwarded your example file and will try to add the issue here when it becomes available.

@senesis
Copy link
Contributor Author

senesis commented Jun 2, 2021

Quick side note on the slow loading: I am just talking to some iris developers and it seems they are hitting the exact same problem, also while dealing with an XIOS generated multi-variable output file for the next model generation. There doesn't seem to be an issue about it yet, but I forwarded your example file and will try to add the issue here when it becomes available.

I reported the problem quite early as an Iris Issue, and provided the file plus a notebook showing the problem by profiling an Iris run, and provided a follow up in my original issue on this subject, which basically says that even with a recent fix in Iris V3.0.2, the load is still slow (figures availabe on demand). Do you mean that the Iris developers have some new idea here ?

@pp-mo
Copy link

pp-mo commented Jun 3, 2021

@senesis even with a recent fix in Iris V3.0.2, the load is still slow

Following discussion with @abooton @zklaus , we've been looking into another possible Iris improvement : to avoid translating data-variables which are not wanted in a load.
A crude demonstration shows a very considerable speedup for loading just one selected cube.

I rather struggled to understand exactly what you need to load here, but I'm assuming may be just a single data-variable,
based on this comment and the CDO use proposed in this PR.
If it's actually more complicated than that, we can probably still adopt the general approach but it will need to be more sophisticated.

@senesis
Copy link
Contributor Author

senesis commented Jun 5, 2021

Commit #896764b bring this PR in line with PR #1124 at commit 50afc8b. I may complement this PR at short notice if needed, including switching to the option of supporting IPSL-CM6 through a dedicated project. I will fix codacy tests diags after the merge of #1124 in main, because a number of them are due to the anticipation on fix.extra_facets existence

@bouweandela
Copy link
Member

including switching to the option of supporting IPSL-CM6 through a dedicated project

That would be good, otherwise you will not be able to use the IPSL model data at the same time as any of the other data supported by the native6 project.

@zklaus zklaus force-pushed the Handle_IPSL_CM6 branch from 0d741c7 to 89f45b9 Compare June 10, 2021 14:05
@zklaus
Copy link

zklaus commented Jun 10, 2021

For now, I just reduced the number of changes by reverting to the previous formatting in unrelated sections. As you can see from the diff, this is much easier to digest. Note that I don't claim that this formatting is better in any way. It just makes it easier to distinguish substantive changes, that we need to proofread, from the rest.

The main things for this were:

  • Remove trailing spaces (perhaps something your editor may support you with)
  • Turn tabs into spaces
  • Restore the original line-breaks where possible

I will now do a proper review of the documentation changes.

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, finished the doc review. Only minor comments.

doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/develop/fixing_data.rst Outdated Show resolved Hide resolved
doc/quickstart/find_data.rst Outdated Show resolved Hide resolved
doc/quickstart/find_data.rst Outdated Show resolved Hide resolved
senesis and others added 10 commits June 10, 2021 17:05
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
Co-authored-by: Klaus Zimmermann <klaus.zimmermann@smhi.se>
@zklaus
Copy link

zklaus commented Jun 10, 2021

I made a small improvement to the formatting.

I also have one more question: The atmospheric variables are now separated according to CMIP vs IPSL names. Does this not apply to the other realms? Also, there are a number of very long #============ lines in the facets file. Do we need them? If you want to keep them, could we shorten them, maybe to 79 chars total line length or similar?

Copy link
Contributor Author

@senesis senesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine

doc/develop/fixing_data.rst Show resolved Hide resolved
@zklaus zklaus dismissed bouweandela’s stale review June 11, 2021 12:58

The review comments were addressed and we @bouweandela and I discussed offline how to move forward.

@zklaus zklaus merged commit 68d9ef8 into main Jun 11, 2021
@zklaus zklaus deleted the Handle_IPSL_CM6 branch June 11, 2021 12:59
@pp-mo

This comment has been minimized.

@senesis

This comment has been minimized.

@pp-mo

This comment has been minimized.

@senesis

This comment has been minimized.

@bouweandela

This comment has been minimized.

@pp-mo

This comment has been minimized.

@zklaus
Copy link

zklaus commented Jun 15, 2021

I have moved the discussion on Iris loading times and CDO to the new issue #1180. Let's continue there.

@senesis
Copy link
Contributor Author

senesis commented Jun 19, 2021

By the way : thanks a lot for the support on this PR and the related issue ; nice to have that yet in V3.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants