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

Added clip_start_end_year preprocessor #796

Merged
merged 12 commits into from
Nov 25, 2020
Merged

Added clip_start_end_year preprocessor #796

merged 12 commits into from
Nov 25, 2020

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Sep 28, 2020

This PR adds an clip_start_end_year preprocessor which is necessary to move the extract_time, extract_month and extract_season preprocessors down in the preprocessor order which allows their use with custom_order: true.

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 #762

@schlunma schlunma added the preprocessor Related to the preprocessor label Sep 28, 2020
@schlunma schlunma self-assigned this Sep 28, 2020
@bouweandela
Copy link
Member

It would be good to take #345 into account, at least for the naming of the new preprocessor function.

@schlunma schlunma changed the title Added extract_years preprocessor Added extract_timerange preprocessor Sep 28, 2020
@schlunma schlunma marked this pull request as ready for review September 28, 2020 15:51
@schlunma
Copy link
Contributor Author

Ready for review

@valeriupredoi
Copy link
Contributor

wait - am thoroughly confused - now we have extract_timerange which is a more specific case of extract_time and they are both preprocessor functions and only extract_timerange is used in the recipe for data filtering, that doesn't support filtering on months, which is needed in cases when we have data ending in weird months like, say, October - @schlunma can you pls tell me the logic behind this - I see both a duplicated functionality and the use of a less performant function at the same time (yeah, am cranky today 😁 ) 🍺 - also, have you changed the doumentation?

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

see me comment above 🍺

@schlunma
Copy link
Contributor Author

Hey V., sorry I somehow didn't post my answer on your question above (which I already typed 😬).

So here is the thing:
extract_timerange should NOT be used by the user, it's only an internal function for extracting the start_year and end_year given by the recipe (similar to other "hidden" preprocessor functions like load or concatenate). That's the reason why we don't need documentation for it. extract_time can now be used by the user and behaves like a "regular" preprocessor now (i.e. it can be shuffled around with custom_order=true and it is not used by default anymore).

I really think that we should merge this soon, other people ran into this issue as well: #762 (comment) and ESMValGroup/ESMValTool#1864 (comment).

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

ok I got persuaded by @schlunma - looks like something more flexible and frontend-y than the internal backend function 👍

@bouweandela
Copy link
Member

@bascrezee @lukasbrunner @Peter9192 This looks like something you need #796 (comment), would one of you be willing to try if it works for your use case?

@schlunma schlunma added this to ESMValCore - needs review in November 2020 Nov 24, 2020
@Peter9192
Copy link
Contributor

Anyone currently reviewing this? Otherwise I can have a go at it

@schlunma
Copy link
Contributor Author

Yes, please have a go at it @Peter9192. Thanks! ☺️

@Peter9192 Peter9192 moved this from ESMValCore - needs review to ESMValCore - in review in November 2020 Nov 24, 2020
@Peter9192 Peter9192 self-requested a review November 24, 2020 14:34
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Code looks good @schlunma ! I left a few comments, will now proceed with some offline testing.
One general question: is extract_timerange really the most suitable name for this preprocessor, or can we come up with something slightly more distinguishing? clip_start_endyear is the best I could come up with so far.

esmvalcore/preprocessor/__init__.py Outdated Show resolved Hide resolved
esmvalcore/preprocessor/__init__.py Outdated Show resolved Hide resolved
esmvalcore/preprocessor/_time.py Outdated Show resolved Hide resolved
tests/integration/test_recipe.py Show resolved Hide resolved
tests/integration/test_recipe.py Show resolved Hide resolved
tests/unit/preprocessor/_time/test_time.py Outdated Show resolved Hide resolved
@Peter9192
Copy link
Contributor

Pytest locally ran fine after merging origin/master into this branch, and I tested with a custom recipe, which also produced the expected results. So I'm quite happy with the code. If you could just address my comments/questions above, then I think we're good to go.

test diagnostic
documentation:
  description: test PR
  authors:
    - kalverla_peter

preprocessors:
  tas_anomalies:
    custom_order: true
    area_statistics:
      operator: mean
    anomalies:
      period: full
      reference:
        start_year: 1981
        start_month: 1
        start_day: 1
        end_year: 2010
        end_month: 12
        end_day: 31
      standardize: false
    extract_time:
      start_year: 2081
      start_month: 1
      start_day: 1
      end_day: 31
      end_month: 12
      end_year: 2099

diagnostics:
  test1:
    variables:
      tas:
        start_year: 1965
        end_year: 2099
        mip: Amon
        preprocessor: tas_anomalies
        additional_datasets:
          - {dataset: ACCESS1-0, project: CMIP5, exp: [historical, rcp85], ensemble: r1i1p1}
    scripts:
      quickplot:
        script: examples/diagnostic.py
        quickplot:
          plot_type: plot

@schlunma
Copy link
Contributor Author

Thanks @Peter9192 for your review! 👍 I included all your comments, so this PR should be ready now.

@Peter9192 Peter9192 changed the title Added extract_timerange preprocessor Added clip_start_end_year preprocessor Nov 25, 2020
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Thanks @schlunma I think you missed one rename, but otherwise I'm happy to see this merged

Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
@schlunma schlunma moved this from ESMValCore - in review to Ready to Merge in November 2020 Nov 25, 2020
@schlunma schlunma merged commit 86fe855 into master Nov 25, 2020
November 2020 automation moved this from Ready to Merge to Merged Nov 25, 2020
@schlunma schlunma deleted the time_preproc branch November 25, 2020 11:09
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
No open projects
Development

Successfully merging this pull request may close these issues.

Preprocessor order: positioning of extract_time
4 participants