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

Apply select preprocessor to area, volume and zonal statistics fx variables and add use of fx_files as list of dictionaries #439

Closed
wants to merge 81 commits into from

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Jan 21, 2020

Before you start, please read CONTRIBUTING.md.

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • This pull request has a descriptive title that can be used in a changelog
  • Add unit tests
  • Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
  • If writing a new/modified preprocessor function, please update the documentation
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • Please use yamllint to check that your YAML files do not contain mistakes
  • If you make backward incompatible changes to the recipe format, make a new pull request in the ESMValTool repository and add the link below

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.


Closes #436 #440

@valeriupredoi
Copy link
Contributor Author

OK @ledm this is now functional, can you give it a test please? To derive fx vars for volume_statistics add fx_var_preprocess: True in variable eg:

        diagnostics:
          diagnostic_name:
            variables:
              tos:
                preprocessor: preproc
                fx_var_preprocess: True
                project: CMIP6
                mip: Omon
                exp: historical
                start_year: 2000
                end_year: 2005
                ensemble: r1i1p1f1
                grid: gn
                additional_datasets:
                  - {dataset: CanESM5}

esmvalcore/_recipe.py Outdated Show resolved Hide resolved
esmvalcore/_recipe.py Outdated Show resolved Hide resolved
@valeriupredoi
Copy link
Contributor Author

aye, finally! This was tested by me with a (rather comperehensive ) recipe:

# recipe_fxprep.yml
---
documentation:

  description: test recipe for fx var preprocessing for area/volume/zonal/statistics

  authors:
    - predoi_valeriu

preprocessors:
    prep:
        custom_order: true
        extract_volume:
            z_min: 0
            z_max: 100
        annual_statistics:
            operator: mean
        volume_statistics:
            operator: mean
            fx_files: [volcello, ]

diagnostics:
  diag:
    variables:
      thetao700_Omon:
        short_name: thetao
        preprocessor: prep
        fx_var_preprocess: True
        additional_datasets:
          - {dataset: GFDL-CM4,        project: CMIP6, mip: Omon, exp: historical, ensemble: r1i1p1f1, grid: gn, start_year: 2010, end_year: 2014}
          - {dataset: HadGEM3-GC31-LL, project: CMIP6, mip: Omon, exp: historical, ensemble: r1i1p1f3, grid: gn, start_year: 2010, end_year: 2014}
          - {dataset: UKESM1-0-LL,     project: CMIP6, mip: Omon, exp: historical, ensemble: r2i1p1f2, grid: gn, start_year: 2010, end_year: 2014}

@mattiarighi and @ledm pls fire up tests yourselves. There may still be corner cases that I haven't looked at but since that recipe above works am quite happy 🍺 I need to add documentation!

@valeriupredoi
Copy link
Contributor Author

Please have a look at this comment #436 (comment) and let's discuss the design there, before continuing.

Please have a look at #436 (comment)

@valeriupredoi
Copy link
Contributor Author

@bouweandela let's get this one reviewed, please mate - we need these features and we need them to work proper and without having to worry about iris changes. These features work in this (rather laborious) way and have been tested quite extensively by @ledm - on top of it all this PR introduces some useful task filters too that I see useful not only for fx variables

@valeriupredoi
Copy link
Contributor Author

OK guys - major plot twist: while trying to fix a bug that @ledm reported yesterday I discovered that there was a major issue with the fx files generation as it was until now - for the sake of not repeating analyses, I was removing ancestors with the same name and leaving just one - but that one was totally unclear with which preprocessing set of steps was being generated with. I fixed this by:

  • not removing any ancestors anymore
  • writing the output preprocessed fx file in with the parent variable data
  • when loading for parent variable happens, loading for the attached fx variable data happens too -> pretty much what iris should be doing (but it doesn't, really)
  • fixed tests too
    🍺

@schlunma schlunma added this to the IPCC AR6 milestone Mar 10, 2020
@valeriupredoi valeriupredoi mentioned this pull request Jun 17, 2020
9 tasks
@valeriupredoi
Copy link
Contributor Author

@ledm are you still using this branch? Can I close and nuke if not?

@valeriupredoi
Copy link
Contributor Author

@ledm are you still using this branch? Can I close and nuke if not?

Frenly ping @ledm - even if you said you're still using it I'd be inclined to close it since fixing those conflicts is reason for a Nobel prize for literature

@ledm
Copy link
Contributor

ledm commented Nov 18, 2021

Lol, this is from feb 2020? What a simple beautiful time that was! We went out for a curry in Reading - we were planning to go to Singapore. Fun times.

I'm working on the main branch and it appears that this branch's functionality is already included there? Either that or I have some workaround in my recipes, but I had assumed that this was already long merged!

@valeriupredoi
Copy link
Contributor Author

ah yes, the good old pre-pandemic days, all a warm blur now 😭

The fx handling functionality and API has changed massively with the introduction of handling of cell measures and variables, thanks to @sloosvel we are now in the 21st century on that. I'll nuke this even though I hope this was useful to you at least before we had the current implements, so farewell a few weeks of coding and sweat 🏳️

@valeriupredoi valeriupredoi deleted the preprocess_fx_variables branch November 18, 2021 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preprocessor chain is not being applied to cell measures
5 participants