diff options
author | Charles Harris <charlesr.harris@gmail.com> | 2020-12-13 14:14:49 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-12-13 14:14:49 -0700 |
commit | 3fe2d9d2627fc0f84aeed293ff8afa7c1f08d899 (patch) | |
tree | 2ea27fe06a19c39e8d7a5fe2f87cb7e05363247d /doc/source/dev | |
parent | 7d7e446fcbeeff70d905bde2eb0264a797488280 (diff) | |
parent | eff302e5e8678fa17fb3d8156d49eb585b0876d9 (diff) | |
download | numpy-3fe2d9d2627fc0f84aeed293ff8afa7c1f08d899.tar.gz |
Merge branch 'master' into fix-issue-10244
Diffstat (limited to 'doc/source/dev')
-rw-r--r-- | doc/source/dev/conduct/code_of_conduct.rst | 163 | ||||
-rw-r--r-- | doc/source/dev/conduct/report_handling_manual.rst | 220 | ||||
-rw-r--r-- | doc/source/dev/development_advanced_debugging.rst | 190 | ||||
-rw-r--r-- | doc/source/dev/development_environment.rst | 4 | ||||
-rw-r--r-- | doc/source/dev/development_workflow.rst | 37 | ||||
-rw-r--r-- | doc/source/dev/index.rst | 32 | ||||
-rw-r--r-- | doc/source/dev/reviewer_guidelines.rst | 119 | ||||
-rw-r--r-- | doc/source/dev/style_guide.rst | 8 |
8 files changed, 368 insertions, 405 deletions
diff --git a/doc/source/dev/conduct/code_of_conduct.rst b/doc/source/dev/conduct/code_of_conduct.rst deleted file mode 100644 index f2f0a536d..000000000 --- a/doc/source/dev/conduct/code_of_conduct.rst +++ /dev/null @@ -1,163 +0,0 @@ -NumPy Code of Conduct -===================== - - -Introduction ------------- - -This code of conduct applies to all spaces managed by the NumPy project, -including all public and private mailing lists, issue trackers, wikis, blogs, -Twitter, and any other communication channel used by our community. The NumPy -project does not organise in-person events, however events related to our -community should have a code of conduct similar in spirit to this one. - -This code of conduct should be honored by everyone who participates in -the NumPy community formally or informally, or claims any affiliation with the -project, in any project-related activities and especially when representing the -project, in any role. - -This code is not exhaustive or complete. It serves to distill our common -understanding of a collaborative, shared environment and goals. Please try to -follow this code in spirit as much as in letter, to create a friendly and -productive environment that enriches the surrounding community. - - -Specific Guidelines -------------------- - -We strive to: - -1. Be open. We invite anyone to participate in our community. We prefer to use - public methods of communication for project-related messages, unless - discussing something sensitive. This applies to messages for help or - project-related support, too; not only is a public support request much more - likely to result in an answer to a question, it also ensures that any - inadvertent mistakes in answering are more easily detected and corrected. - -2. Be empathetic, welcoming, friendly, and patient. We work together to resolve - conflict, and assume good intentions. We may all experience some frustration - from time to time, but we do not allow frustration to turn into a personal - attack. A community where people feel uncomfortable or threatened is not a - productive one. - -3. Be collaborative. Our work will be used by other people, and in turn we will - depend on the work of others. When we make something for the benefit of the - project, we are willing to explain to others how it works, so that they can - build on the work to make it even better. Any decision we make will affect - users and colleagues, and we take those consequences seriously when making - decisions. - -4. Be inquisitive. Nobody knows everything! Asking questions early avoids many - problems later, so we encourage questions, although we may direct them to - the appropriate forum. We will try hard to be responsive and helpful. - -5. Be careful in the words that we choose. We are careful and respectful in - our communication and we take responsibility for our own speech. Be kind to - others. Do not insult or put down other participants. We will not accept - harassment or other exclusionary behaviour, such as: - - - Violent threats or language directed against another person. - - Sexist, racist, or otherwise discriminatory jokes and language. - - Posting sexually explicit or violent material. - - Posting (or threatening to post) other people's personally identifying information ("doxing"). - - Sharing private content, such as emails sent privately or non-publicly, - or unlogged forums such as IRC channel history, without the sender's consent. - - Personal insults, especially those using racist or sexist terms. - - Unwelcome sexual attention. - - Excessive profanity. Please avoid swearwords; people differ greatly in their sensitivity to swearing. - - Repeated harassment of others. In general, if someone asks you to stop, then stop. - - Advocating for, or encouraging, any of the above behaviour. - - -Diversity Statement -------------------- - -The NumPy project welcomes and encourages participation by everyone. We are -committed to being a community that everyone enjoys being part of. Although -we may not always be able to accommodate each individual's preferences, we try -our best to treat everyone kindly. - -No matter how you identify yourself or how others perceive you: we welcome you. -Though no list can hope to be comprehensive, we explicitly honour diversity in: -age, culture, ethnicity, genotype, gender identity or expression, language, -national origin, neurotype, phenotype, political beliefs, profession, race, -religion, sexual orientation, socioeconomic status, subculture and technical -ability, to the extent that these do not conflict with this code of conduct. - - -Though we welcome people fluent in all languages, NumPy development is -conducted in English. - -Standards for behaviour in the NumPy community are detailed in the Code of -Conduct above. Participants in our community should uphold these standards -in all their interactions and help others to do so as well (see next section). - - -Reporting Guidelines --------------------- - -We know that it is painfully common for internet communication to start at or -devolve into obvious and flagrant abuse. We also recognize that sometimes -people may have a bad day, or be unaware of some of the guidelines in this Code -of Conduct. Please keep this in mind when deciding on how to respond to a -breach of this Code. - -For clearly intentional breaches, report those to the Code of Conduct committee -(see below). For possibly unintentional breaches, you may reply to the person -and point out this code of conduct (either in public or in private, whatever is -most appropriate). If you would prefer not to do that, please feel free to -report to the Code of Conduct Committee directly, or ask the Committee for -advice, in confidence. - -You can report issues to the NumPy Code of Conduct committee, at -numpy-conduct@googlegroups.com. Currently, the committee consists of: - -- Stefan van der Walt -- Melissa Weber Mendonça -- Anirudh Subramanian - -If your report involves any members of the committee, or if they feel they have -a conflict of interest in handling it, then they will recuse themselves from -considering your report. Alternatively, if for any reason you feel -uncomfortable making a report to the committee, then you can also contact: - -- Senior `NumFOCUS staff <https://numfocus.org/code-of-conduct#persons-responsible>`__: conduct@numfocus.org - - -Incident reporting resolution & Code of Conduct enforcement ------------------------------------------------------------ - -*This section summarizes the most important points, more details can be found -in* :ref:`CoC_reporting_manual`. - -We will investigate and respond to all complaints. The NumPy Code of Conduct -Committee and the NumPy Steering Committee (if involved) will protect the -identity of the reporter, and treat the content of complaints as confidential -(unless the reporter agrees otherwise). - -In case of severe and obvious breaches, e.g. personal threat or violent, sexist -or racist language, we will immediately disconnect the originator from NumPy -communication channels; please see the manual for details. - -In cases not involving clear severe and obvious breaches of this code of -conduct, the process for acting on any received code of conduct violation -report will be: - -1. acknowledge report is received -2. reasonable discussion/feedback -3. mediation (if feedback didn't help, and only if both reporter and reportee agree to this) -4. enforcement via transparent decision (see :ref:`CoC_resolutions`) by the - Code of Conduct Committee - -The committee will respond to any report as soon as possible, and at most -within 72 hours. - - -Endnotes --------- - -We are thankful to the groups behind the following documents, from which we -drew content and inspiration: - -- `The SciPy Code of Conduct <https://docs.scipy.org/doc/scipy/reference/dev/conduct/code_of_conduct.html>`_ - diff --git a/doc/source/dev/conduct/report_handling_manual.rst b/doc/source/dev/conduct/report_handling_manual.rst deleted file mode 100644 index d39b615bb..000000000 --- a/doc/source/dev/conduct/report_handling_manual.rst +++ /dev/null @@ -1,220 +0,0 @@ -:orphan: - -.. _CoC_reporting_manual: - -NumPy Code of Conduct - How to follow up on a report ----------------------------------------------------- - -This is the manual followed by NumPy's Code of Conduct Committee. It's used -when we respond to an issue to make sure we're consistent and fair. - -Enforcing the Code of Conduct impacts our community today and for the future. -It's an action that we do not take lightly. When reviewing enforcement -measures, the Code of Conduct Committee will keep the following values and -guidelines in mind: - -* Act in a personal manner rather than impersonal. The Committee can engage - the parties to understand the situation, while respecting the privacy and any - necessary confidentiality of reporters. However, sometimes it is necessary - to communicate with one or more individuals directly: the Committee's goal is - to improve the health of our community rather than only produce a formal - decision. - -* Emphasize empathy for individuals rather than judging behavior, avoiding - binary labels of "good" and "bad/evil". Overt, clear-cut aggression and - harassment exists and we will be address that firmly. But many scenarios - that can prove challenging to resolve are those where normal disagreements - devolve into unhelpful or harmful behavior from multiple parties. - Understanding the full context and finding a path that re-engages all is - hard, but ultimately the most productive for our community. - -* We understand that email is a difficult medium and can be isolating. - Receiving criticism over email, without personal contact, can be - particularly painful. This makes it especially important to keep an - atmosphere of open-minded respect of the views of others. It also means - that we must be transparent in our actions, and that we will do everything - in our power to make sure that all our members are treated fairly and with - sympathy. - -* Discrimination can be subtle and it can be unconscious. It can show itself - as unfairness and hostility in otherwise ordinary interactions. We know - that this does occur, and we will take care to look out for it. We would - very much like to hear from you if you feel you have been treated unfairly, - and we will use these procedures to make sure that your complaint is heard - and addressed. - -* Help increase engagement in good discussion practice: try to identify where - discussion may have broken down and provide actionable information, pointers - and resources that can lead to positive change on these points. - -* Be mindful of the needs of new members: provide them with explicit support - and consideration, with the aim of increasing participation from - underrepresented groups in particular. - -* Individuals come from different cultural backgrounds and native languages. - Try to identify any honest misunderstandings caused by a non-native speaker - and help them understand the issue and what they can change to avoid causing - offence. Complex discussion in a foreign language can be very intimidating, - and we want to grow our diversity also across nationalities and cultures. - -*Mediation*: voluntary, informal mediation is a tool at our disposal. In -contexts such as when two or more parties have all escalated to the point of -inappropriate behavior (something sadly common in human conflict), it may be -useful to facilitate a mediation process. This is only an example: the -Committee can consider mediation in any case, mindful that the process is meant -to be strictly voluntary and no party can be pressured to participate. If the -Committee suggests mediation, it should: - -* Find a candidate who can serve as a mediator. -* Obtain the agreement of the reporter(s). The reporter(s) have complete - freedom to decline the mediation idea, or to propose an alternate mediator. -* Obtain the agreement of the reported person(s). -* Settle on the mediator: while parties can propose a different mediator than - the suggested candidate, only if common agreement is reached on all terms can - the process move forward. -* Establish a timeline for mediation to complete, ideally within two weeks. - -The mediator will engage with all the parties and seek a resolution that is -satisfactory to all. Upon completion, the mediator will provide a report -(vetted by all parties to the process) to the Committee, with recommendations -on further steps. The Committee will then evaluate these results (whether -satisfactory resolution was achieved or not) and decide on any additional -action deemed necessary. - - -How the committee will respond to reports -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -When the committee (or a committee member) receives a report, they will first -determine whether the report is about a clear and severe breach (as defined -below). If so, immediate action needs to be taken in addition to the regular -report handling process. - -Clear and severe breach actions -+++++++++++++++++++++++++++++++ - -We know that it is painfully common for internet communication to start at or -devolve into obvious and flagrant abuse. We will deal quickly with clear and -severe breaches like personal threats, violent, sexist or racist language. - -When a member of the Code of Conduct committee becomes aware of a clear and -severe breach, they will do the following: - -* Immediately disconnect the originator from all NumPy communication channels. -* Reply to the reporter that their report has been received and that the - originator has been disconnected. -* In every case, the moderator should make a reasonable effort to contact the - originator, and tell them specifically how their language or actions - qualify as a "clear and severe breach". The moderator should also say - that, if the originator believes this is unfair or they want to be - reconnected to NumPy, they have the right to ask for a review, as below, by - the Code of Conduct Committee. - The moderator should copy this explanation to the Code of Conduct Committee. -* The Code of Conduct Committee will formally review and sign off on all cases - where this mechanism has been applied to make sure it is not being used to - control ordinary heated disagreement. - -Report handling -+++++++++++++++ - -When a report is sent to the committee they will immediately reply to the -reporter to confirm receipt. This reply must be sent within 72 hours, and the -group should strive to respond much quicker than that. - -If a report doesn't contain enough information, the committee will obtain all -relevant data before acting. The committee is empowered to act on the Steering -Council’s behalf in contacting any individuals involved to get a more complete -account of events. - -The committee will then review the incident and determine, to the best of their -ability: - -* What happened. -* Whether this event constitutes a Code of Conduct violation. -* Who are the responsible party(ies). -* Whether this is an ongoing situation, and there is a threat to anyone's - physical safety. - -This information will be collected in writing, and whenever possible the -group's deliberations will be recorded and retained (i.e. chat transcripts, -email discussions, recorded conference calls, summaries of voice conversations, -etc). - -It is important to retain an archive of all activities of this committee to -ensure consistency in behavior and provide institutional memory for the -project. To assist in this, the default channel of discussion for this -committee will be a private mailing list accessible to current and future -members of the committee as well as members of the Steering Council upon -justified request. If the Committee finds the need to use off-list -communications (e.g. phone calls for early/rapid response), it should in all -cases summarize these back to the list so there's a good record of the process. - -The Code of Conduct Committee should aim to have a resolution agreed upon within -two weeks. In the event that a resolution can't be determined in that time, the -committee will respond to the reporter(s) with an update and projected timeline -for resolution. - - -.. _CoC_resolutions: - -Resolutions -~~~~~~~~~~~ - -The committee must agree on a resolution by consensus. If the group cannot reach -consensus and deadlocks for over a week, the group will turn the matter over to -the Steering Council for resolution. - - -Possible responses may include: - -* Taking no further action - - - if we determine no violations have occurred. - - if the matter has been resolved publicly while the committee was considering responses. - -* Coordinating voluntary mediation: if all involved parties agree, the - Committee may facilitate a mediation process as detailed above. -* Remind publicly, and point out that some behavior/actions/language have been - judged inappropriate and why in the current context, or can but hurtful to - some people, requesting the community to self-adjust. -* A private reprimand from the committee to the individual(s) involved. In this - case, the group chair will deliver that reprimand to the individual(s) over - email, cc'ing the group. -* A public reprimand. In this case, the committee chair will deliver that - reprimand in the same venue that the violation occurred, within the limits of - practicality. E.g., the original mailing list for an email violation, but - for a chat room discussion where the person/context may be gone, they can be - reached by other means. The group may choose to publish this message - elsewhere for documentation purposes. -* A request for a public or private apology, assuming the reporter agrees to - this idea: they may at their discretion refuse further contact with the - violator. The chair will deliver this request. The committee may, if it - chooses, attach "strings" to this request: for example, the group may ask a - violator to apologize in order to retain one’s membership on a mailing list. -* A "mutually agreed upon hiatus" where the committee asks the individual to - temporarily refrain from community participation. If the individual chooses - not to take a temporary break voluntarily, the committee may issue a - "mandatory cooling off period". -* A permanent or temporary ban from some or all NumPy spaces (mailing lists, - gitter.im, etc.). The group will maintain records of all such bans so that - they may be reviewed in the future or otherwise maintained. - -Once a resolution is agreed upon, but before it is enacted, the committee will -contact the original reporter and any other affected parties and explain the -proposed resolution. The committee will ask if this resolution is acceptable, -and must note feedback for the record. - -Finally, the committee will make a report to the NumPy Steering Council (as -well as the NumPy core team in the event of an ongoing resolution, such as a -ban). - -The committee will never publicly discuss the issue; all public statements will -be made by the chair of the Code of Conduct Committee or the NumPy Steering -Council. - - -Conflicts of Interest -~~~~~~~~~~~~~~~~~~~~~ - -In the event of any conflict of interest, a committee member must immediately -notify the other members, and recuse themselves if necessary. diff --git a/doc/source/dev/development_advanced_debugging.rst b/doc/source/dev/development_advanced_debugging.rst new file mode 100644 index 000000000..fa4014fdb --- /dev/null +++ b/doc/source/dev/development_advanced_debugging.rst @@ -0,0 +1,190 @@ +======================== +Advanced debugging tools +======================== + +If you reached here, you want to dive into, or use, more advanced tooling. +This is usually not necessary for first time contributers and most +day-to-day developement. +These are used more rarely, for example close to a new NumPy release, +or when a large or particular complex change was made. + +Since not all of these tools are used on a regular bases and only available +on some systems, please expect differences, issues, or quirks; +we will be happy to help if you get stuck and appreciate any improvements +or suggestions to these workflows. + + +Finding C errors with additional tooling +######################################## + +Most development will not require more than a typical debugging toolchain +as shown in :ref:`Debugging <debugging>`. +But for example memory leaks can be particularly subtle or difficult to +narrow down. + +We do not expect any of these tools to be run by most contributors. +However, you can ensure that we can track down such issues more easily easier: + +* Tests should cover all code paths, incluing error paths. +* Try to write short and simple tests. If you have a very complicated test + consider creating an additional simpler test as well. + This can be helpful, because often it is only easy to find which test + triggers an issue and not which line of the test. +* Never use ``np.empty`` if data is read/used. ``valgrind`` will notice this + and report an error. When you do not care about values, you can generate + random values instead. + +This will help us catch any oversights before your change is released +and means you do not have to worry about making reference counting errors, +which can be intimidating. + + +Python debug build for finding memory leaks +=========================================== + +Debug builds of Python are easily available for example on ``debian`` systems, +and can be used on all platforms. +Running a test or terminal is usually as easy as:: + + python3.8d runtests.py + # or + python3.8d runtests.py --ipython + +and were already mentioned in :ref:`Debugging <debugging>`. + +A Python debug build will help: + +- Find bugs which may otherwise cause random behaviour. + One example is when an object is still used after it has been deleted. + +- Python debug builds allows to check correct reference counting. + This works using the additional commands:: + + sys.gettotalrefcount() + sys.getallocatedblocks() + + +Use together with ``pytest`` +---------------------------- + +Running the test suite only with a debug python build will not find many +errors on its own. An additional advantage of a debug build of Python is that +it allows detecting memory leaks. + +A tool to make this easier is `pytest-leaks`_, which can be installed using ``pip``. +Unfortunately, ``pytest`` itself may leak memory, but good results can usually +(currently) be achieved by removing:: + + @pytest.fixture(autouse=True) + def add_np(doctest_namespace): + doctest_namespace['np'] = numpy + + @pytest.fixture(autouse=True) + def env_setup(monkeypatch): + monkeypatch.setenv('PYTHONHASHSEED', '0') + +from ``numpy/conftest.py`` (This may change with new ``pytest-leaks`` versions +or ``pytest`` updates). + +This allows to run the test suite, or part of it, conveniently:: + + python3.8d runtests.py -t numpy/core/tests/test_multiarray.py -- -R2:3 -s + +where ``-R2:3`` is the ``pytest-leaks`` command (see its documentation), the +``-s`` causes output to print and may be necessary (in some versions captured +output was detected as a leak). + +Note that some tests are known (or even designed) to leak references, we try +to mark them, but expect some false positives. + +.. _pytest-leaks: https://github.com/abalkin/pytest-leaks + +``valgrind`` +============ + +Valgrind is a powerful tool to find certain memory access problems and should +be run on complicated C code. +Basic use of ``valgrind`` usually requires no more than:: + + PYTHONMALLOC=malloc python runtests.py + +where ``PYTHONMALLOC=malloc`` is necessary to avoid false positives from python +itself. +Depending on the system and valgrind version, you may see more false positives. +``valgrind`` supports "suppressions" to ignore some of these, and Python does +have a supression file (and even a compile time option) which may help if you +find it necessary. + +Valgrind helps: + +- Find use of uninitialized variables/memory. + +- Detect memory access violations (reading or writing outside of allocated + memory). + +- Find *many* memory leaks. Note that for *most* leaks the python + debug build approach (and ``pytest-leaks``) is much more sensitive. + The reason is that ``valgrind`` can only detect if memory is definitely + lost. If:: + + dtype = np.dtype(np.int64) + arr.astype(dtype=dtype) + + Has incorrect reference counting for ``dtype``, this is a bug, but valgrind + cannot see it because ``np.dtype(np.int64)`` always returns the same object. + However, not all dtypes are singletons, so this might leak memory for + different input. + In rare cases NumPy uses ``malloc`` and not the Python memory allocators + which are invisible to the Python debug build. + ``malloc`` should normally be avoided, but there are some exceptions + (e.g. the ``PyArray_Dims`` structure is public API and cannot use the + Python allocators.) + +Even though using valgrind for memory leak detection is slow and less sensitive +it can be a convenient: you can run most programs with valgrind without +modification. + +Things to be aware of: + +- Valgrind does not support the numpy ``longdouble``, this means that tests + will fail or be flagged errors that are completely fine. + +- Expect some errors before and after running your NumPy code. + +- Caches can mean that errors (specifically memory leaks) may not be detected + or are only detect at a later, unrelated time. + +A big advantage of valgrind is that it has no requirements aside from valgrind +itself (although you probably want to use debug builds for better tracebacks). + + +Use together with ``pytest`` +---------------------------- +You can run the test suite with valgrind which may be sufficient +when you are only interested in a few tests:: + + PYTHOMMALLOC=malloc valgrind python runtests.py \ + -t numpy/core/tests/test_multiarray.py -- --continue-on-collection-errors + +Note the ``--continue-on-collection-errors``, which is currently necessary due to +missing ``longdouble`` support causing failures (this will usually not be +necessary if you do not run the full test suite). + +If you wish to detect memory leaks you will also require ``--show-leak-kinds=definite`` +and possibly more valgrind options. Just as for ``pytest-leaks`` certain +tests are known to leak cause errors in valgrind and may or may not be marked +as such. + +We have developed `pytest-valgrind`_ which: + +- Reports errors for each test individually + +- Narrows down memory leaks to individual tests (by default valgrind + only checks for memory leaks after a program stops, which is very + cumbersome). + +Please refer to its ``README`` for more information (it includes an example +command for NumPy). + +.. _pytest-valgrind: https://github.com/seberg/pytest-valgrind + diff --git a/doc/source/dev/development_environment.rst b/doc/source/dev/development_environment.rst index ff78cecc5..013414568 100644 --- a/doc/source/dev/development_environment.rst +++ b/doc/source/dev/development_environment.rst @@ -207,6 +207,8 @@ repo, use one of:: $ git reset --hard +.. _debugging: + Debugging --------- @@ -273,7 +275,7 @@ pull requests aren't perfect, the community is always happy to help. As a volunteer project, things do sometimes get dropped and it's totally fine to ping us if something has sat without a response for about two to four weeks. -So go ahead and pick something that annoys or confuses you about numpy, +So go ahead and pick something that annoys or confuses you about NumPy, experiment with the code, hang around for discussions or go through the reference documents to try to fix it. Things will fall in place and soon you'll have a pretty good understanding of the project as a whole. Good Luck! diff --git a/doc/source/dev/development_workflow.rst b/doc/source/dev/development_workflow.rst index d5a49a9f9..34535b2f5 100644 --- a/doc/source/dev/development_workflow.rst +++ b/doc/source/dev/development_workflow.rst @@ -188,6 +188,16 @@ Standard acronyms to start the commit message with are:: REL: related to releasing numpy +.. _workflow_mailing_list: + +Get the mailing list's opinion +======================================================= + +If you plan a new feature or API change, it's wisest to first email the +NumPy `mailing list <https://mail.python.org/mailman/listinfo/numpy-discussion>`_ +asking for comment. If you haven't heard back in a week, it's +OK to ping the list again. + .. _asking-for-merging: Asking for your changes to be merged with the main repo @@ -197,15 +207,24 @@ When you feel your work is finished, you can create a pull request (PR). Github has a nice help page that outlines the process for `filing pull requests`_. If your changes involve modifications to the API or addition/modification of a -function, you should +function, add a release note to the ``doc/release/upcoming_changes/`` +directory, following the instructions and format in the +``doc/release/upcoming_changes/README.rst`` file. + + +.. _workflow_PR_timeline: + +Getting your PR reviewed +======================== + +We review pull requests as soon as we can, typically within a week. If you get +no review comments within two weeks, feel free to ask for feedback by +adding a comment on your PR (this will notify maintainers). + +If your PR is large or complicated, asking for input on the numpy-discussion +mailing list may also be useful. + -- send an email to the `NumPy mailing list`_ with a link to your PR along with - a description of and a motivation for your changes. This may generate - changes and feedback. It might be prudent to start with this step if your - change may be controversial. -- add a release note to the ``doc/release/upcoming_changes/`` directory, - following the instructions and format in the - ``doc/release/upcoming_changes/README.rst`` file. .. _rebasing-on-master: @@ -290,7 +309,7 @@ Rewriting commit history Do this only for your own feature branches. -There's an embarrassing typo in a commit you made? Or perhaps the you +There's an embarrassing typo in a commit you made? Or perhaps you made several false starts you would like the posterity not to see. This can be done via *interactive rebasing*. diff --git a/doc/source/dev/index.rst b/doc/source/dev/index.rst index aeb277a87..bcd144d71 100644 --- a/doc/source/dev/index.rst +++ b/doc/source/dev/index.rst @@ -4,6 +4,22 @@ Contributing to NumPy ##################### +.. TODO: this is hidden because there's a bug in the pydata theme that won't render TOC items under headers + +.. toctree:: + :hidden: + + Git Basics <gitwash/index> + development_environment + development_workflow + development_advanced_debugging + ../benchmarking + NumPy C style guide <https://numpy.org/neps/nep-0045-c_style_guide.html> + releasing + governance/index + howto-docs + + Not a coder? Not a problem! NumPy is multi-faceted, and we can use a lot of help. These are all activities we'd like to get help with (they're all important, so we list them in alphabetical order): @@ -107,7 +123,8 @@ Here's the short summary, complete TOC links are below: overall code quality benefits. Therefore, please don't let the review discourage you from contributing: its only aim is to improve the quality of project, not to criticize (we are, after all, very grateful for the - time you're donating!). + time you're donating!). See our :ref:`Reviewer Guidelines + <reviewer-guidelines>` for more information. * To update your PR, make your changes on your local repository, commit, **run tests, and only if they succeed** push to your fork. As soon as @@ -164,6 +181,8 @@ be merged automatically, you have to incorporate changes that have been made since you started into your branch. Our recommended way to do this is to :ref:`rebase on master<rebasing-on-master>`. +.. _guidelines: + Guidelines ---------- @@ -171,9 +190,11 @@ Guidelines * All code should be `documented <https://numpydoc.readthedocs.io/ en/latest/format.html#docstring-standard>`_. * No changes are ever committed without review and approval by a core - team member.Please ask politely on the PR or on the `mailing list`_ if you + team member. Please ask politely on the PR or on the `mailing list`_ if you get no response to your pull request within a week. +.. _stylistic-guidelines: + Stylistic Guidelines -------------------- @@ -218,6 +239,8 @@ This will create a report in ``build/coverage``, which can be viewed with:: $ firefox build/coverage/index.html +.. _building-docs: + Building docs ------------- @@ -277,12 +300,13 @@ The rest of the story .. toctree:: :maxdepth: 2 - conduct/code_of_conduct Git Basics <gitwash/index> development_environment development_workflow + development_advanced_debugging + reviewer_guidelines ../benchmarking - style_guide + NumPy C style guide <https://numpy.org/neps/nep-0045-c_style_guide.html> releasing governance/index howto-docs diff --git a/doc/source/dev/reviewer_guidelines.rst b/doc/source/dev/reviewer_guidelines.rst new file mode 100644 index 000000000..0b225b9b6 --- /dev/null +++ b/doc/source/dev/reviewer_guidelines.rst @@ -0,0 +1,119 @@ +.. _reviewer-guidelines: + +=================== +Reviewer Guidelines +=================== + +Reviewing open pull requests (PRs) helps move the project forward. We encourage +people outside the project to get involved as well; it's a great way to get +familiar with the codebase. + +Who can be a reviewer? +====================== + +Reviews can come from outside the NumPy team -- we welcome contributions from +domain experts (for instance, `linalg` or `fft`) or maintainers of other +projects. You do not need to be a NumPy maintainer (a NumPy team member with +permission to merge a PR) to review. + +If we do not know you yet, consider introducing yourself in `the mailing list or +Slack <https://numpy.org/community/>`_ before you start reviewing pull requests. + +Communication Guidelines +======================== + +- Every PR, good or bad, is an act of generosity. Opening with a positive + comment will help the author feel rewarded, and your subsequent remarks may be + heard more clearly. You may feel good also. +- Begin if possible with the large issues, so the author knows they've been + understood. Resist the temptation to immediately go line by line, or to open + with small pervasive issues. +- You are the face of the project, and NumPy some time ago decided `the kind of + project it will be <https://numpy.org/code-of-conduct/>`_: open, empathetic, + welcoming, friendly and patient. Be `kind + <https://youtu.be/tzFWz5fiVKU?t=49m30s>`_ 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 on + style or grammar, consider merging the current PR when all important concerns + are 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 some `Standard replies + for reviewing + <https://scikit-learn.org/stable/developers/tips.html#saved-replies>`_. + +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 be `int` or `tuples`? + - Are unusual `dtypes` 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 the :ref:`NumPy guidelines<howto-document>`? Are + the docstrings properly formatted? +- Does the code follow NumPy's :ref:`Stylistic Guidelines<stylistic-guidelines>`? +- If you are a maintainer, and it is not obvious from the PR description, add a + short explanation of what a branch did to the merge message and, if closing an + issue, 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 a + PR and approve of the changes use the GitHub `approve review + <https://help.github.com/articles/reviewing-changes-in-pull-requests/>`_ tool + to mark it as such. If a PR is straightforward, for example it's a clearly + correct bug fix, it can be merged straight away. If it's more complex or + changes public API, please leave it open for at least a couple of days so + other maintainers get a chance to review. +- If you are a subsequent reviewer on an already approved PR, please use the + same review method as for a new PR (focus on the larger issues, resist the + temptation to add only a few nitpicks). If you have commit rights and think + no more review is needed, merge the PR. + +For maintainers +--------------- + +- Make sure all automated CI tests pass before merging a PR, and that the + :ref:`documentation builds <building-docs>` without any errors. +- In case of merge conflicts, ask the PR submitter to :ref:`rebase on master + <rebasing-on-master>`. +- For PRs that add new features or are in some way complex, wait at least a day + or two before merging it. That way, others get a chance to comment before the + code goes in. Consider adding it to the release notes. +- When merging contributions, a committer is responsible for ensuring that those + meet the requirements outlined in the :ref:`Development process guidelines + <guidelines>` for NumPy. Also, check that new features and backwards + compatibility breaks were discussed on the `numpy-discussion mailing list + <https://mail.python.org/mailman/listinfo/numpy-discussion>`_. +- Squashing commits or cleaning up commit messages of a PR that you consider too + messy is OK. Remember to retain the original author's name when doing this. + Make sure commit messages follow the :ref:`rules for NumPy + <writing-the-commit-message>`. +- When you want to reject a PR: if it's very obvious, you can just close it and + explain why. If it's not, then it's a good idea to first explain why you + think the PR is not suitable for inclusion in NumPy and then let a second + committer comment or close. + +GitHub Workflow +--------------- + +When reviewing pull requests, please use workflow tracking features on GitHub as +appropriate: + +- After you have finished reviewing, if you want to ask for the submitter to + make changes, change your review status to "Changes requested." This can be + done on GitHub, PR page, Files changed tab, Review changes (button on the top + right). +- If you're happy about the current status, mark the pull request as Approved + (same way as Changes requested). Alternatively (for maintainers): merge + the 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 your +own machine so that you can play with it locally. You can use the `GitHub CLI +<https://docs.github.com/en/github/getting-started-with-github/github-cli>`_ to +do this by clicking the ``Open with`` button in the upper right-hand corner of +the PR page. + +Assuming you have your :ref:`development environment<development-environment>` +set up, you can now build the code and test it. + +.. include:: gitwash/git_links.inc diff --git a/doc/source/dev/style_guide.rst b/doc/source/dev/style_guide.rst deleted file mode 100644 index bede3424d..000000000 --- a/doc/source/dev/style_guide.rst +++ /dev/null @@ -1,8 +0,0 @@ -.. _style_guide: - -=================== -NumPy C Style Guide -=================== - -.. include:: ../../C_STYLE_GUIDE.rst.txt - :start-line: 4 |