Reviewer guidelines#

Reviewing open pull requests (PRs) helps move the project forward. We encouragepeople outside the project to get involved as well; it’s a great way to getfamiliar with the codebase.

Who can be a reviewer?#

Reviews can come from outside the NumPy team – we welcome contributions fromdomain experts (for instance,linalg orfft) or maintainers of otherprojects. You do not need to be a NumPy maintainer (a NumPy team member withpermission to merge a PR) to review.

If we do not know you yet, consider introducing yourself inthe mailing list orSlack before you start reviewing pull requests.

Communication guidelines#

  • Every PR, good or bad, is an act of generosity. Opening with a positivecomment will help the author feel rewarded, and your subsequent remarks may beheard more clearly. You may feel good also.

  • Begin if possible with the large issues, so the author knows they’ve beenunderstood. Resist the temptation to immediately go line by line, or to openwith small pervasive issues.

  • You are the face of the project, and NumPy some time ago decidedthe kind ofproject it will be: open, empathetic,welcoming, friendly and patient. Bekind to contributors.

  • Do not let perfect be the enemy of the good, particularly for documentation.If you find yourself making many small suggestions, or being too nitpicky onstyle or grammar, consider merging the current PR when all important concernsare addressed. Then, either push a commit directly (if you are a maintainer)or open a follow-up PR yourself.

  • If you need help writing replies in reviews, check out somestandard replies for reviewing.

Reviewer checklist#

  • Is the intended behavior clear under all conditions? Some things to watch:

    • What happens with unexpected inputs like empty arrays or nan/inf values?

    • Are axis or shape arguments tested to beint ortuples?

    • Are unusualdtypes tested if a function supports those?

  • Should variable names be improved for clarity or consistency?

  • Should comments be added, or rather removed as unhelpful or extraneous?

  • Does the documentation follow theNumPy guidelines? Arethe docstrings properly formatted?

  • Does the code follow NumPy’sStylistic Guidelines?

  • If you are a maintainer, and it is not obvious from the PR description, add ashort explanation of what a branch did to the merge message and, if closing anissue, also add “Closes gh-123” where 123 is the issue number.

  • For code changes, at least one maintainer (i.e. someone with commit rights)should review and approve a pull request. If you are the first to review aPR and approve of the changes use the GitHubapprove review toolto mark it as such. If a PR is straightforward, for example it’s a clearlycorrect bug fix, it can be merged straight away. If it’s more complex orchanges public API, please leave it open for at least a couple of days soother maintainers get a chance to review.

  • If you are a subsequent reviewer on an already approved PR, please use thesame review method as for a new PR (focus on the larger issues, resist thetemptation to add only a few nitpicks). If you have commit rights and thinkno more review is needed, merge the PR.

For maintainers#

  • Make sure all automated CI tests pass before merging a PR, and that thedocumentation builds without any errors.

  • In case of merge conflicts, ask the PR submitter torebase on main.

  • For PRs that add new features or are in some way complex, wait at least a dayor two before merging it. That way, others get a chance to comment before thecode goes in. Consider adding it to the release notes.

  • When merging contributions, a committer is responsible for ensuring that thosemeet the requirements outlined in theDevelopment process guidelines for NumPy. Also, check that new features and backwardscompatibility breaks were discussed on thenumpy-discussion mailing list.

  • Squashing commits or cleaning up commit messages of a PR that you consider toomessy is OK. Remember to retain the original author’s name when doing this.Make sure commit messages follow therules for NumPy.

  • When you want to reject a PR: if it’s very obvious, you can just close it andexplain why. If it’s not, then it’s a good idea to first explain why youthink the PR is not suitable for inclusion in NumPy and then let a secondcommitter comment or close.

  • If the PR submitter doesn’t respond to your comments for 6 months, move the PRin question to the inactive category with the “inactive” tag attached.At this point, the PR can be closed by a maintainer. If there is any interestin finalizing the PR under consideration, this can be indicated at any time,without waiting 6 months, by a comment.

  • Maintainers are encouraged to finalize PRs when only small changes arenecessary before merging (e.g., fixing code style or grammatical errors).If a PR becomes inactive, maintainers may make larger changes.Remember, a PR is a collaboration between a contributor and a reviewer/s,sometimes a direct push is the best way to finish it.

API changes#

As mentioned most public API changes should be discussed ahead of time andoften with a wider audience (on the mailing list, or even through a NEP).

For changes in the public C-API be aware that the NumPy C-API is backwardscompatible so that any addition must be ABI compatible with previous versions.When it is not the case, you must add a guard.

For examplePyUnicodeScalarObject struct contains the following:

#if NPY_FEATURE_VERSION >= NPY_1_20_API_VERSIONchar*buffer_fmt;#endif

Because thebuffer_fmt field was added to its end in NumPy 1.20 (allprevious fields remained ABI compatible).Similarly, any function added to the API table innumpy/_core/code_generators/numpy_api.py must use theMinVersionannotation.For example:

'PyDataMem_SetHandler':(304,MinVersion("1.22")),

Header only functionality (such as a new macro) typically does not need to beguarded.

GitHub workflow#

When reviewing pull requests, please use workflow tracking features on GitHub asappropriate:

  • After you have finished reviewing, if you want to ask for the submitter tomake changes, change your review status to “Changes requested.” This can bedone on GitHub, PR page, Files changed tab, Review changes (button on the topright).

  • If you’re happy about the current status, mark the pull request as Approved(same way as Changes requested). Alternatively (for maintainers): mergethe pull request, if you think it is ready to be merged.

It may be helpful to have a copy of the pull request code checked out on yourown machine so that you can play with it locally. You can use theGitHub CLI todo this by clicking theOpenwith button in the upper right-hand corner ofthe PR page.

Assuming you have yourdevelopment environmentset up, you can now build the code and test it.

Standard replies for reviewing#

It may be helpful to store some of these in GitHub’ssavedreplies for reviewing:

Usage question
You are asking a usage question. The issue tracker is for bugs and new features.I'm going to close this issue, feel free to ask for help via our [help channels](https://numpy.org/gethelp/).
You’re welcome to update the docs
Please feel free to offer a pull request updating the documentation if you feel it could be improved.
Self-contained example for bug
Please provide a [self-contained example code](https://stackoverflow.com/help/mcve), including imports and data (if possible), so that other contributors can just run it and reproduce your issue.Ideally your example code should be minimal.
Software versions
To help diagnose your issue, please paste the output of:```python -c 'import numpy; print(numpy.version.version)'```Thanks.
Code blocks
Readability can be greatly improved if you [format](https://help.github.com/articles/creating-and-highlighting-code-blocks/) your code snippets and complete error messages appropriately.You can edit your issue descriptions and comments at any time to improve readability.This helps maintainers a lot. Thanks!
Linking to code
For clarity's sake, you can link to code like [this](https://help.github.com/articles/creating-a-permanent-link-to-a-code-snippet/).
Better description and title
Please make the title of the PR more descriptive.The title will become the commit message when this is merged.You should state what issue (or PR) it fixes/resolves in the description using the syntax described [here](https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword).
Regression test needed
Please add a [non-regression test](https://en.wikipedia.org/wiki/Non-regression_testing) that would fail at main but pass in this PR.
Don’t change unrelated
Please do not change unrelated lines. It makes your contribution harder to review and may introduce merge conflicts to other pull requests.