Skip to content

Updated docstrings for area and volume statistics to work with fx variables definitions (PR 439 child 3)#509

Closed
valeriupredoi wants to merge 2 commits intomasterfrom
PR439_child3_area_volume_doc
Closed

Updated docstrings for area and volume statistics to work with fx variables definitions (PR 439 child 3)#509
valeriupredoi wants to merge 2 commits intomasterfrom
PR439_child3_area_volume_doc

Conversation

@valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented Feb 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.


READ BEFORE PROCEEDING

Issue #436 lays down the need for preprocessing fx variables as integrated with area_atatistics and volume_statistics; PR #439 implements that; reviewers expressed their consternation that that PR is huuuge so it got broken down; the layout for the PR 439 children is outlined in #436 (comment)
This is the 3rd child that depends upon integration of the 2nd child; it contains the updated docstrings for area and volume statistics in light of the new recipe syntax with fx vars as dicts.

Copy link

@zklaus zklaus left a comment

Choose a reason for hiding this comment

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

Looks ok, but is this linked to code changes as well? I think this update might need to go together with the corresponding code changes or at least be linked with the relevant PR so that we can check that the description reflects the behaviour.

@valeriupredoi
Copy link
Contributor Author

yes, we should not merge now, lemme just create the order in which the three PRs should be examined and merged. Gimme a sec, will tag you in one moment 🍺

@mattiarighi mattiarighi added documentation Improvements or additions to documentation preprocessor Related to the preprocessor labels Feb 26, 2020
@schlunma schlunma added this to the IPCC AR6 milestone Mar 10, 2020
@schlunma
Copy link
Contributor

schlunma commented May 4, 2021

I believe we can close this since #999 got merged, can't we?

@zklaus
Copy link

zklaus commented May 5, 2021

Agreed. #999 removed the parameters whose documentation is improved here. Closing. @valeriupredoi please reopen if you disagree.

@zklaus zklaus closed this May 5, 2021
@bouweandela bouweandela deleted the PR439_child3_area_volume_doc branch May 5, 2021 09:09
@bouweandela
Copy link
Member

Deleted the branch to keep the repo tidy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation preprocessor Related to the preprocessor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants