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

Fix overwriting in generic level check #886

Merged
merged 5 commits into from
Dec 2, 2020
Merged

Conversation

sloosvel
Copy link
Contributor

@sloosvel sloosvel commented Nov 30, 2020

Before you start, please read our contribution guidelines.

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

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


The previous implementation was working directly with the dict containing the CMOR information. This led to accidental overwritting of the dict values in some runs and ended up causing incorrect reporting of errors. Tested with several datasets and variables and it seems that this issue does not happen anymore, but should be double checked of course.

Closes #883

@sloosvel sloosvel requested review from jvegreg and schlunma November 30, 2020 10:59
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.

nice work @sloosvel 🍺 @schlunma would you be able to test again for the cases reported previously - cheers guys!

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Works perfectly fine now, thanks @sloosvel for the quick fix! 👍 I tested this with around 40 CMIP5 and CMIP6 models (let the recipe run for ~10 times) with no issues.

I have one small optimization for the code.

@valeriupredoi
Copy link
Contributor

@sloosvel and @schlunma yous are legends, cheers for the quick turnaround. Saskia, let me know when you ready with the suggestion from Manuel and I can merge - I think this is a very nice fix and I would like to include this in the bugfix release that @bouweandela and myself will do tomorrow so it'd be nice if you addressed Manu's suggestion pronto-pronto 😁

Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
@bouweandela
Copy link
Member

@valeriupredoi There is no need to rush this, as the bug is not in a released version.

@bouweandela bouweandela added bug Something isn't working cmor Related to the CMOR standard labels Dec 1, 2020
@valeriupredoi
Copy link
Contributor

ah right you are @bouweandela - I was under the impression we've released it already - ok, then we can wait, sorry for hurrying you @sloosvel 😀

@bouweandela bouweandela merged commit 5e91b2b into master Dec 2, 2020
@bouweandela bouweandela deleted the dev_fix_level_check branch December 2, 2020 16:38
@bouweandela bouweandela changed the title Fix overwritting in generic level check Fix overwriting in generic level check Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cmor Related to the CMOR standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic level checks lead to inconsistent ESMValTool runs
4 participants