-
Notifications
You must be signed in to change notification settings - Fork 52
MPAS Analysis pull requests
Give the pull request (PR) a short, descriptive title. If the PR is a work in progress (not very close to being ready to merge but needing discussion), start the title with "WIP:" and add the "work in progress" tag.
Describe the PR in language appropriate for a git commit message. Do not describe tests performed, just the functionality to be merged. Leave out discussion items, example figures, etc.
In a comment titled "Testing" (or similar), describe which tests have been performed that give you confidence that the PR functions as expected.
If the PR is intended to address a GitHub issue, link to the issue number using #<issue_number>. For example, "This PR is intended to address #256".
Add reviewer(s) who should look over the code and/or perform tests. Indicate to the reviewer(s) what kind of input is required from them.
Assign one reviewer (or yourself) under Assignees. This person will merge the code in the end.
The code should follow the PEP8 standard as closely as possible. Either run the flake8 tool to see which lines are not PEP8 compliant and why or use an integrated development environment (e.g. spyder) that is aware of PEP8. To turn on PEP8 checking in spyder, go to Preferences -> Editor -> Code Introspection/Analysis and select the tickbox next to realtime code style analysis. If this checkbox is grayed out, make sure to install the pep8 package (via conda install).
Each PR that adds functionality should make sure the new functionality gets added to the documentation (in the docs/ folder). For now, we only have auto-generated documentation, meaning new modules (that is, new python files), new functions and new classes need to get added to docs/api.rst. Please follow the example of the existing functionality.
Once the new hooks are added, make sure the documentation is generated correctly (without errors) using:
make html
in the docs/ directory. You can look at the results in the _html folder.
Make sure continuous integration (CI) tests pass successfully. This happens automatically each time you update the remote branch associated with the PR. If CI has failed, you will see this indicated at the bottom of the PR (e.g. "All checks have failed" with a red x). If CI has failed, this means that one or more of the tests in the test/ directory has failed and needs to be debugged. Run:
pytest
or
pytest mpas_analysis/tests/test_analysis_task.py
to see what has gone wrong.
It is a good idea to add unit tests to any functionality (other than plotting functions) that you add to the the shared code in mpas_analysis/shared. Take a look at the existing tests and ask for help on how to do this.
Typically, unit tests on analysis tasks are not possible because the amount of data and computation time needed to perform meaningful tests is too great. Instead, we perform manual testing on all or part of MPAS-Analysis as part of each PR (see below).
If a review is requested of you, please inform the requester as soon as you are able if you cannot review and remove yourself as a reviewer.
Assuming you are able to review, you may be asked to look over the code and/or run some tests. Either the submitter or one or more reviewers should run the Standard Tests (see below) and, if possible, share the resulting HTML output as part of the PR.
Comment, approve or request changes to the PR following these instructions.
The PR should not be merged until all reviewers (who have not removed themselves for lack of time, interest or expertise) have approved the PR.
For each PR, either the submitter or a reviewer should run each of the tests defined by the following config files, verifying that they run without errors and that the resulting webpage shows the expected plots (e.g. visually identical to previous results):
- On Edison:
configs/edison/config.20170313.beta1.A_WCYCL1850S.ne30_oECv3_ICG.edisonconfigs/edison/config.20170807.beta1.G_oQU240.edison
- On Titan or Rhea:
configs/olcf/config.GMPAS-IAF_oRRS18to6v3.titan
See the subdirectories for each machine under configs for more details on how to run analysis in batch mode on each machine.
If for some reason one or more of these tests cannot be run before merging a PR, this should be noted in the Testing comment.
Before merging develop to master to create a new tag, MPAS-Analysis and A-Prime should be tested on all machines supported by E3SM. A full list of config files for these tests have not been collected but will be added to the MPAS-Analysis and/or A-Prime repo in the near future.