Skip to content

Contributing guidelines improvements #495

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

Merged
merged 5 commits into from
Jul 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Contributing
============

We welcome your contributions! Please see the [contributing](http://pvlib-python.readthedocs.io/en/latest/contributing.html) page for information about how to contribute.
16 changes: 9 additions & 7 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
pvlib python pull request guidelines
====================================

Thank you for your contribution to pvlib python!
Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:
You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

- [ ] Closes issue #xxxx
- [ ] Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
- [ ] Code quality and style is sufficient. Passes ``git diff upstream/master -u -- "*.py" | flake8 --diff`` and/or landscape.io linting service.
- [ ] New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
- [ ] I am familiar with the [contributing guidelines](http://pvlib-python.readthedocs.io/en/latest/contributing.html).
- [ ] Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
- [ ] Updates entries to `docs/sphinx/source/api.rst` for API changes.
- [ ] Adds description and name entries in the appropriate `docs/sphinx/source/whatsnew` file for all changes.

Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.
- [ ] Code quality and style is sufficient. Passes ``git diff upstream/master -u -- "*.py" | flake8 --diff``
- [ ] New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
- [ ] Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
223 changes: 206 additions & 17 deletions docs/sphinx/source/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,27 @@ Encouraging more people to help develop pvlib-python is essential to our
success. Therefore, we want to make it easy and rewarding for you to
contribute.

There is a lot of material in this section, aimed at a variety of
contributors from novice to expert. Don't worry if you don't (yet)
understand parts of it.


Easy ways to contribute
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the word "how" is missing on line 18: " Here are a few ideas for you can contribute, ..."

~~~~~~~~~~~~~~~~~~~~~~~

Here are a few ideas for you can contribute, even if you are new to
Here are a few ideas for how you can contribute, even if you are new to
pvlib-python, git, or Python:

* Ask and answer `pvlib questions on StackOverflow <http://stackoverflow.com/questions/tagged/pvlib>`_
and participate in discussions in the `pvlib-python google group <https://groups.google.com/forum/#!forum/pvlib-python>`_.
* Make `GitHub issues <https://github.com/pvlib/pvlib-python/issues>`_
and contribute to the conversation about how to resolve them.
and contribute to the conversations about how to resolve them.
* Read issues and pull requests that other people created and
contribute to the conversation about how to resolve them.
Look for issues tagged with
`good first issue <https://github.com/pvlib/pvlib-python/labels/good%20first%20issue>`_,
`easy <https://github.com/pvlib/pvlib-python/labels/easy>`_,
or `help wanted <https://github.com/pvlib/pvlib-python/labels/help%20wanted>`_.
* Improve the documentation and the unit tests.
* Improve the IPython/Jupyter Notebook tutorials or write new ones that
demonstrate how to use pvlib-python in your area of expertise.
Expand All @@ -35,6 +45,9 @@ pvlib-python, git, or Python:
How to contribute new code
~~~~~~~~~~~~~~~~~~~~~~~~~~

The basics
----------

Contributors to pvlib-python use GitHub's pull requests to add/modify
its source code. The GitHub pull request process can be intimidating for
new users, but you'll find that it becomes straightforward once you use
Expand All @@ -53,16 +66,11 @@ process. Here's an outline of the process:
request.

The Pandas project maintains an excellent `contributing page
<https://github.com/pydata/pandas/wiki/Contributing>`_ that goes into
detail on each of these steps. Also see GitHub's `Set Up Git
<http://pandas.pydata.org/pandas-docs/stable/contributing.html>`_ that goes
into detail on each of these steps. Also see GitHub's `Set Up Git
<https://help.github.com/articles/set-up-git/>`_ and `Using Pull
Requests <https://help.github.com/articles/using-pull-requests/>`_.

Note that you do not need to make all of your changes before creating a
pull request. Your pull requests will automatically be updated when you
commit new changes and push them to GitHub. This gives everybody an easy
way to comment on the code and can make the process more efficient.

We strongly recommend using virtual environments for development.
Virtual environments make it trivial to switch between different
versions of software. This `astropy guide
Expand All @@ -73,17 +81,187 @@ environment.

You must include documentation and unit tests for any new or improved
code. We can provide help and advice on this after you start the pull
request.
request. See the Testing section below.


.. _pull-request-scope:

Pull request scope
------------------

This section can be summed up as "less is more".

A pull request can quickly become unmanageable if too many lines are
added or changed. "Too many" is hard to define, but as a rule of thumb,
we encourage contributions that contain less than 50 lines of primary code.
50 lines of primary code will typically need at least 250 lines
of documentation and testing. This is about the limit of what the
maintainers can review on a regular basis.

A pull request can also quickly become unmanageable if it proposes
changes to the API in order to implement another feature. Consider
clearly and concisely documenting all proposed API changes before
implementing any code. Modifying
`api.rst <https://github.com/pvlib/pvlib-python/blob/master/docs/sphinx/source/api.rst>`_
and/or the latest `whatsnew file <https://github.com/pvlib/pvlib-python/tree/master/docs/sphinx/source/whatsnew>`_
can help formalize this process.

Questions about related issues frequently come up in the process of
addressing implementing code for a pull request. Please try to avoid
expanding the scope of your pull request (this also applies to
reviewers!). We'd rather see small, well-documented additions to the
project's technical debt than see a pull request languish because its
scope expanded beyond what the reviewer community is capable of
processing.

Of course, sometimes it is necessary to make a large pull request. We
only ask that you take a few minutes to consider how to break it into
smaller chunks before proceeding.

pvlib-python contains :ref:`3 "layers" of code <modeling-paradigms>`:
functions, PVSystem/Location, and ModelChain. We recommend that
contributors focus their work on only one or two of those layers in a
single pull request. New models are *not* required to be available to
the higher-level API!
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to see a ton of value in adding models that aren't accessible to (perhaps many?) users through the higher-level API. Perhaps touch on this point here: What is the value and/or subsequent steps to make a new model "fully" accessible?

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with what Will wrote here. At the last workshop we heard from a broad sample of users that pvlib (either MATLAB or python) serves two primary roles: as a reference library of algorithms (our original intent), and as a platform for research or custom modeling to supplement commercial applications. ModelChain is valuable, although not essential, to these use cases. So it's OK, from my point of view, to add model functions and expose them at the PVSystem/Location level but not the ModelChain level.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Cliff said it well. I'd add that I'm not sure that ModelChain is going to exist in anything like its current form in 3-5 years, but I am sure that most of the core functions will remain fairly similar to what they are now.



When should I submit a pull request?
------------------------------------

The short answer: anytime.

The long answer: it depends. If in doubt, go ahead and submit. You do
not need to make all of your changes before creating a pull request.
Your pull requests will automatically be updated when you commit new
changes and push them to GitHub.

There are pros and cons to submitting incomplete pull-requests. On the
plus side, it gives everybody an easy way to comment on the code and can
make the process more efficient. On the minus side, it's easy for an
incomplete pull request to grow into a multi-month saga that leaves
everyone unhappy. If you submit an incomplete pull request, please be
very clear about what you would like feedback on and what we should
ignore. Alternatives to incomplete pull requests include creating a
`gist <https://gist.github.com>`_ or experimental branch and linking to
it in the corresponding issue.

The best way to ensure that a pull request will be reviewed and merged in
a timely manner is to:

#. Start by creating an issue. The issue should be well-defined and
actionable.
#. Ask the maintainers to tag the issue with the appropriate milestone.
#. Make a limited-scope pull request. It can be a lot of work to check all of
the boxes in `pull request guidelines
<https://github.com/pvlib/pvlib-python/blob/master/.github/PULL_REQUEST_TEMPLATE.md>`_,
especially for pull requests with a lot of new primary code.
See :ref:`pull-request-scope`.
#. Tag pvlib community members or ``@pvlib/maintainer`` when the pull
request is ready for review. (see :ref:`pull-request-reviews`)


.. _pull-request-reviews:

Pull request reviews
--------------------

The pvlib community and maintainers will review your pull request in a
timely fashion. Please "ping" ``@pvlib/maintainer`` if it seems that
your pull request has been forgotten at any point in the pull request
process.

Keep in mind that the PV modeling community is diverse and each pvlib
community member brings a different perspective when reviewing code.
Some reviewers bring years of expertise in the sub-field that your code
contributes to and will focus on the details of the algorithm. Other
reviewers will be more focused on integrating your code with the rest of
pvlib, ensuring that it is feasible to maintain, that it meets the
:ref:`code style <code-style>` guidelines, and that it is
:ref:`comprehensively tested <testing>`. Limiting the scope of the pull
request makes it much more likely that all of these reviews can be
conducted and any issues can be resolved in a timely fashion.

Sometimes it's hard for reviewers to be immediately available, so the
right amount of patience is to be expected. That said, interested
reviewers should do their best to not wait until the last minute to put
in their two cents.


.. _code-style:

Code style
~~~~~~~~~~

pvlib python generally follows the `PEP 8 -- Style Guide for Python Code
<https://www.python.org/dev/peps/pep-0008/>`_. Maximum line length for code
is 79 characters.

Code must be compatible with python 2.7 and 3.4+.

pvlib python uses a mix of full and abbreviated variable names. See
:ref:`variables_style_rules`. We could be better about consistency.
Prefer full names for new contributions. This is especially important
for the API. Abbreviations can be used within a function to improve the
readability of formulae.

Set your editor to strip extra whitespace from line endings. This
prevents the git commit history from becoming cluttered with whitespace
changes.

Please see :ref:`Documentation` for information specific to documentation
style.

Remove any ``logging`` calls and ``print`` statements that you added
during development. ``warning`` is ok.

We typically use GitHub's
"`squash and merge` <https://help.github.com/articles/about-pull-request-merges/#squash-and-merge-your-pull-request-commits>_"
feature to merge your pull request into pvlib. GitHub will condense the
commit history of your branch into a single commit when merging into
pvlib-python/master (the commit history on your branch remains
unchanged). Therefore, you are free to make commits that are as big or
small as you'd like while developing your pull request.


.. _documentation:

The maintainers will follow same procedures, rather than making direct
commits to the main repo. Exceptions may be made for extremely minor
changes, such as fixing documentation typos.
Documentation
~~~~~~~~~~~~~

Documentation must be written in
`numpydoc format <https://numpydoc.readthedocs.io/>`_.

The numpydoc format includes a specification for the allowable input
types. Python's `duck typing <https://en.wikipedia.org/wiki/Duck_typing>`_
allows for multiple input types to work for many parameters. pvlib uses
the following generic descriptors as short-hand to indicate which
specific types may be used:

* dict-like : dict, OrderedDict, pd.Series
* numeric : scalar, np.array, pd.Series. Typically int or float dtype.
* array-like : np.array, pd.Series. Typically int or float dtype.

Parameters that specify a specific type require that specific input type.

A relatively easy way to test your documentation is to build it on
`readthedocs.org <https://readthedocs.org>` by following their
`Import Your Docs <http://docs.readthedocs.io/en/stable/getting_started.html#import-your-docs>`_
instructions and enabling your branch on the readthedocs
`versions admin page <http://docs.readthedocs.io/en/stable/features.html#versions>`_.

Another option is to install the required dependencies in your virtual/conda
environment. See
`docs/environment.yml <https://github.com/pvlib/pvlib-python/blob/master/docs/environment.yml>`_
for the latest dependences for building the complete documentation. Some
doc files can be compiled with fewer dependencies, but this is beyond
the scope of this guide.

.. _testing:

Testing
~~~~~~~

pvlib's unit tests can easily be run by executing ``py.test`` on the
pvlib's unit tests can easily be run by executing ``pytest`` on the
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 196 below says "avoid using print. Do you mean don't commit code with print statements, or is there a better way to log extra info when a test fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 206 should replace "layer" by "layer(s)", because a change could have testing ramifications in multiple layers.

Copy link
Member Author

Choose a reason for hiding this comment

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

clarified and fixed.

pvlib directory:

``pytest pvlib``
Expand All @@ -96,7 +274,16 @@ or, for a single test:

``pytest pvlib/test/test_clearsky.py::test_ineichen_nans``

Use the ``--pdb`` flag to debug failures and avoid using ``print``.
We suggest using pytest's ``--pdb`` flag to debug test failures rather
than using ``print`` or ``logging`` calls. For example:

``pytest pvlib --pdb``

will drop you into the
`pdb debugger <https://docs.python.org/3/library/pdb.html>`_ at the
location of a test failure. As described in :ref:`code-style`, pvlib
code does not use ``print`` or ``logging`` calls, and this also applies
to the test suite (with rare exceptions).

New unit test code should be placed in the corresponding test module in
the pvlib/test directory.
Expand All @@ -106,7 +293,7 @@ modifications to pvlib.

pvlib-python contains 3 "layers" of code: functions, PVSystem/Location,
and ModelChain. Contributors will need to add tests that correspond to
the layer that they modify.
the layers that they modify.

Functions
---------
Expand Down Expand Up @@ -148,6 +335,7 @@ in fact, happen. Finally, we check that the output of the method call is
reasonable.

.. code-block:: python

def test_PVSystem_ashraeiam(mocker):
# mocker is a pytest-mock object.
# mocker.spy adds code to a function to keep track of how it is called
Expand Down Expand Up @@ -189,6 +377,7 @@ The example below shows how mock can be used to assert that the correct
PVSystem method is called through ``ModelChain.run_model``.

.. code-block:: python

def test_modelchain_dc_model(mocker):
# set up location and system for model chain
location = location.Location(32, -111)
Expand Down Expand Up @@ -220,5 +409,5 @@ This documentation

If this documentation is unclear, help us improve it! Consider looking
at the `pandas
documentation <http://pandas.pydata.org/pandas-docs/version/0.18.1/
documentation <http://pandas.pydata.org/pandas-docs/stable/
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between "stable" and "latest"? It seems that sometimes "latest" refers to master, but other times to the latest tag, which might not actually be the highest tag. (For example, Docker has some subtleties here, if I recall correctly.)

Copy link
Member Author

Choose a reason for hiding this comment

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

most python projects configure readthedocs such that stable is the most recently released version (git tag), whereas latest uses the current state of the master branch.

contributing.html>`_ for inspiration.
6 changes: 2 additions & 4 deletions docs/sphinx/source/package_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,8 @@ change something about pvlib, then please make an issue on our

How do I contribute?
--------------------
We're so glad you asked! Please see our
`wiki <https://github.com/pvlib/pvlib-python/wiki/Contributing-to-pvlib-python>`_
for information and instructions on how to contribute.
We really appreciate it!
We're so glad you asked! Please see :ref:`Contributing` for information and
instructions on how to contribute. We really appreciate it!


Credits
Expand Down
1 change: 1 addition & 0 deletions docs/sphinx/source/whatsnew/v0.6.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Documentation
objects, and ModelChain.
* Updated several incorrect statements in ModelChain documentation regarding
implementation status and default values. (:issue:`480`)
* Expanded general contributing and pull request guidelines.


Testing
Expand Down