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

Development cmip6fx conventions with cmor fixes to fx vars #22

Closed
wants to merge 60 commits into from

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jun 10, 2019

Related issue #109
ported ESMValGroup/ESMValTool#1037 to ESMValCore/version2_development

@valeriupredoi valeriupredoi added preprocessor Related to the preprocessor paper labels Jun 10, 2019
@zklaus
Copy link

zklaus commented Jun 11, 2019

This contains a lot of stuff about different preprocessors that has nothing to do with fx vars. Did something in the porting go wrong or am I missing something?

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Jun 11, 2019 via email

@zklaus
Copy link

zklaus commented Jun 11, 2019

Ok, I will wait with the review then until this is done 'cause at the moment it is a bit difficult to see what should actually be commented upon.

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented Jun 11, 2019 via email

@bouweandela
Copy link
Member

Can you please merge the development branch into this branch, so we can see what's new?

@valeriupredoi
Copy link
Contributor Author

Can you please merge the development branch into this branch, so we can see what's new?

le done 🍺

This was referenced Jun 14, 2019
@valeriupredoi
Copy link
Contributor Author

@ledm @zklaus @bouweandela I have had another look at this PR and I think it is very important to have it examined, suggestions plugged in and merged on an asap level: this is helping with a bunch of rather important standing issues. Here is an example for the recipe bit:

    variables:
      tos:
        preprocessor: pp_rad
        mip: Omon
        grid: gn
        fx_files: [{short_name: areacello, mip: Ofx}]
      areacello:
        preprocessor: pp_rad
        grid: gn
        mip: Ofx

the fx_files from tos are found, priority-based (preprocessed before tos) CMOR-checked and fixed and then fed to the preprocessor functions that need them (derive, mask and statistics); they are also included in the fx_files dictionary in metadata.yml for diag purposes. NOTE that the preprocessing of fx_files variables is limited to a basic (default) preprocessor because the derive and mask preproc functions need them in their basic form (and not preprocessed); only the area and volume statistics functions would need them preprocessed to various levels.

Also, part of this PR allows for specifying the fx variable like a regular variable, together with any sort of preprocessor for it. This can be used later on in the diagnostic.

NOTE that due to the internal structure of the current workflow, inter-using stand-alone variables with other variables is an absolute nightmare (the parallel nature of running each variable task makes things very tricky). Unless we seriously change the framework, the coupling var-fxvar (or for the matter, var-var) in the preprocessor can not be done unless the fxvar(fx_files) is a subsection of the variable.

@ledm
Copy link
Contributor

ledm commented Jun 20, 2019

only the area and volume statistics functions would need them preprocessed to various levels.

  1. Any preprocessor that includes regridding at an early stage would likely need to regrid the area/volume as well.
  2. Aside from regridding, we can solve some of this problem by changing the extract_region preprocessor from a slice to a mask. (NB It already applies mask if the grid is irregular, it's only regular grids that get the slice!)

@zklaus
Copy link

zklaus commented Jun 20, 2019

@valeriupredoi, nice to see this moving forward!

I can't speak for the others, but what makes the review difficult for me is that it is really unclear what this PR is trying to achieve, what other PRs on the topic are around, and how they relate to each other.
It certainly is difficult to read the history in this case.

How about this: Let's make one issue for fx stuff (or designate an existing one) and collect links to all the bits and pieces there. Then we can clarify what does what.

What do you think?

@valeriupredoi
Copy link
Contributor Author

@zklaus sounds like a very good idea! This PR is too large to be reviewed just using the (bits of) information here. I'll create the issue right now, and ask you guys to contribute 🍺

@valeriupredoi valeriupredoi mentioned this pull request Jun 20, 2019
@valeriupredoi
Copy link
Contributor Author

#109

@valeriupredoi
Copy link
Contributor Author

superseded for now by #170

@valeriupredoi valeriupredoi mentioned this pull request Aug 8, 2019
@valeriupredoi valeriupredoi deleted the development_cmip6fxConventions branch November 7, 2019 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants