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 preprocessor mask_multimodel #767

Merged
merged 7 commits into from
Mar 18, 2021
Merged

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Sep 1, 2020

This PR adds a working multimodel masking preprocessor.

The following things need to be implemented before the draft status can be cleared:

  • Add documentation (and remove the old, wrong parts)
  • Add unit tests
  • Discuss about the name of the preprocessor, I do not think that mask_multimodel is the optimal choice.
  • Adapt provenance of multimodel output

I probably do not have time to finish this soon, so any contributions to this PR are welcome!

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 #487.

Copy link
Contributor

@bascrezee bascrezee left a comment

Choose a reason for hiding this comment

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

To me these changes look good. I also tested this preprocessor quite thoroughly by now, and it works as expected.

@valeriupredoi
Copy link
Contributor

@schlunma you wanna switch this PR from Draft to RfR, mate? I'll haz a look right after you did that 👍

@schlunma
Copy link
Contributor Author

I still need to add tests and documentation for this, will do that this week/next week. After that I will switch the status of this PR 👍

@valeriupredoi
Copy link
Contributor

I still need to add tests and documentation for this, will do that this week/next week. After that I will switch the status of this PR +1

rock on, dude! 🤘

@schlunma schlunma marked this pull request as ready for review March 18, 2021 14:20
@schlunma
Copy link
Contributor Author

This is ready for review now 👍

I added unit tests and documentation and also tested it on real data with the recipe mentioned in #487. All works as expected.

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.

looks good! a couple minor points inline, but also - could you pls add an integration test inside test_recipe where you call the preprocessor and do some stuff with it? Cheers, dude 🍺

esmvalcore/preprocessor/_mask.py Outdated Show resolved Hide resolved
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.

excellent stuff, cheers much @schlunma 🍺

@schlunma schlunma merged commit d50452c into master Mar 18, 2021
@schlunma schlunma deleted the preproc_mask_multimodel branch March 18, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New preprocessor mask_multimodel
3 participants