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

Move cmor_check_data to early in preprocessing chain #743

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Aug 6, 2020

Moving the CMOR check of the data to earlier in the default preprocessor order, will make it possible to implement preprocessor functions that change the units of the data without using a custom_order preprocessor. People seem to run into this again and again.

Related to #708, #442, #60

Closes #741, closes #212

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
  • 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.

@bouweandela bouweandela added the cmor Related to the CMOR standard label Aug 6, 2020
@hb326
Copy link
Contributor

hb326 commented Aug 7, 2020

I ran my recipe again that gave me problems in the earlier test (#727) with the new order of preprocessing. It ran through, but the units were still not correct. For the anomaly plot I would expect something with unit=1, but I get the unit of the variable that I use in the recipe.
lineplot_pr_1983-2016

@bascrezee
Copy link
Contributor

bascrezee commented Aug 7, 2020

@hb326 Since the units are set to 1 in another branch ( std-anoms-are-unitless) you have to use that one to see effect. I now merged preproc-check-order such that you can check the combined behaviour. Can you try it again? With the branch std-anoms-are-unitless? @bouweandela Thanks!

@bascrezee
Copy link
Contributor

Succesfully tested in combination with #727.

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.

Looks good to me!

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.

Already tested in the anomalies branch

@jvegreg jvegreg merged commit 594cb65 into master Aug 17, 2020
@jvegreg jvegreg deleted the preproc-check-order branch August 17, 2020 09:41
@valeriupredoi valeriupredoi added the preprocessor Related to the preprocessor label Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmor Related to the CMOR standard preprocessor Related to the preprocessor
Projects
None yet
5 participants