add get_[relative_airmass,sky_diffuse,ground_diffuse,extra_radiation,total_irradiance] and pvlibDeprecationWarning

[wholmgren]

• Closes irradiance.globalinplane appears redundant with total_irrad #422
• 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.
• 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.

This PR implements the refactoring discussed in #422.

I added a deprecation warning to globalinplane.

We could go ahead and change the name of total_irrad in this PR, but that would require changes to a few other parts of the library that I wasn't yet ready to commit to. If we do change it, we'd keep the old name until 0.6 and add a deprecation warning in the mean time (like I did for globalinplane).

Everything here is open for discussion.

[markcampanelli]

I'm not very familiar with this code, but I had a few comments/suggestions.

[markcampanelli]

Is this a good opportunity to make naming more consistent, such as irradiance.extra_radiation and irradiance.ground_diffuse?

[wholmgren]

Sure. Probably a good idea to make all API changes to a module at once.

[cwhanse]

How about the pattern of get_extraterrestrial and get_ground_diffuse as the wrapper, and ground_diffuse_isotropic as our one and only wrapped function? That's what I'm suggesting for sky_diffuse models. I can see the same pattern working for get_clearsky (kwarg to specify the clearsky model), get_airmass, get_iam, etc.

Many function names were simply carried over from Matlab where we wrote a library, not an object-organized package.

[markcampanelli]

Perhaps naming consistency is better here. Only one function has get_ prefix.

[wholmgren]

In this module, yes, but we use get_ for a few other "wrapper" functions/methods in solarposition.py, location.py, and pvsystem.py. I am torn on this.

[cwhanse]

I'm not in favor of 'get_isotropic', an exception to my preference for verb prefixes. isotropic, perez, haydavies are named models. If we want to extend the function name, I'd use a category prefix, i.e., sky_diffuse_isotropic, sky_diffuse_perez, etc., and have get_sky_diffuse wrap all of them with a kwarg.

[markcampanelli]

Does this test essentially check that poa_components() executes without exception?

[wholmgren]

Correct. I only changed this test because I changed the globalinplane function signature to *args so that it could be a direct pass through to poa_components. No one ever bothered to add numeric assertions to this test.

[markcampanelli]

Is there an issue for test improvement that we could put in a comment here?

[wholmgren]

Here are some variations of @cwhanse and @thunderfish24 API suggestions.

I implemented get_total_poa_irradiance, but get_poa_irradiance_total is arguably more similar to get_poa_sky_diffuse and get_poa_ground_diffuse.

get_poa_sky_diffuse or get_sky_diffuse?

get_poa_ground_diffuse or get_ground_diffuse?

Summary in code form:

In [1]: from pvlib import irradiance

In [2]: irradiance.extraradiation(1)
/Users/holmgren/miniconda3/envs/pvlib36/bin/ipython:1: PVLibDeprecationWarning: The extraradiation function was deprecated in version 0.5.2. Use get_extra_radiation instead.
  #!/Users/holmgren/miniconda3/envs/pvlib36/bin/python
Out[2]: 1413.981805

In [3]: irradiance.get_extra_radiation(1)
Out[3]: 1413.981805

In [4]: irradiance.grounddiffuse(30, 100)
/Users/holmgren/miniconda3/envs/pvlib36/bin/ipython:1: PVLibDeprecationWarning: The grounddiffuse function was deprecated in version 0.5.2. Use get_poa_ground_diffuse instead.
  #!/Users/holmgren/miniconda3/envs/pvlib36/bin/python
Out[4]: 1.674682452694516

In [5]: irradiance.get_poa_ground_diffuse(30, 100)
Out[5]: 1.674682452694516

In [6]: irradiance.globalinplane(0, 0, 0, 0)
/Users/holmgren/miniconda3/envs/pvlib36/bin/ipython:1: PVLibDeprecationWarning: The globalinplane function was deprecated in version 0.5.2. Use poa_components instead. This function will be removed in 0.6
  #!/Users/holmgren/miniconda3/envs/pvlib36/bin/python
Out[6]: OrderedDict([('poa_global', 0.0), ('poa_direct', 0.0), ('poa_diffuse', 0)])

In [7]: irradiance.poa_components(0, 0, 0, 0)
Out[7]:
OrderedDict([('poa_global', 0.0),
             ('poa_direct', 0.0),
             ('poa_diffuse', 0),
             ('poa_sky_diffuse', 0),
             ('poa_ground_diffuse', 0)])

In [8]: irradiance.total_irrad(30, 180, 60, 180, 1000, 800, 200)
/Users/holmgren/miniconda3/envs/pvlib36/bin/ipython:1: PVLibDeprecationWarning: The total_irrad function was deprecated in version 0.5.2. Use get_total_poa_irradiance instead.
  #!/Users/holmgren/miniconda3/envs/pvlib36/bin/python
Out[8]:
OrderedDict([('poa_global', 1066.0254037844388),
             ('poa_direct', 866.0254037844387),
             ('poa_diffuse', 200.0),
             ('poa_sky_diffuse', 186.60254037844388),
             ('poa_ground_diffuse', 13.397459621556129)])

In [9]: irradiance.get_total_poa_irradiance(30, 180, 60, 180, 1000, 800, 200)
Out[9]:
OrderedDict([('poa_global', 1066.0254037844388),
             ('poa_direct', 866.0254037844387),
             ('poa_diffuse', 200.0),
             ('poa_sky_diffuse', 186.60254037844388),
             ('poa_ground_diffuse', 13.397459621556129)])

Note that I added matplotlib's deprecation machinery to avoid copy/paste and lots of manual warnings. Credit to Unidata/MetPy#519 for the idea. It looks like AstroPy also uses a version of the matplotlib deprecation machinery. scikit-learn does something similar, but with different code. I don't know if there's a preferred way, but this works reasonably well.

Thoughts?

edited because I botched the MetPy issue number.

[wholmgren]

It might also make sense to change relativeairmass to get_relative_airmass at the same time. And absoluteairmass to absolute_airmass

[cwhanse]

I implemented get_total_poa_irradiance, but get_poa_irradiance_total is arguably more similar to get_poa_sky_diffuse and get_poa_ground_diffuse.
get_poa_sky_diffuse or get_sky_diffuse?
get_poa_ground_diffuse or get_ground_diffuse?

I think the 'poa' is not necessary in these function names, since one can pass tilt=0.

It might also make sense to change relativeairmass to get_relative_airmass at the same time. And absoluteairmass to absolute_airmass

Both make sense to me.

[wholmgren]

Good point. I removed poa from the function names. I also changed the airmass function names as proposed.

I'm building the new API docs here

To do:

• update the tests to use the new function names.
• update some doc strings that reference the wrong function.
• add tests that assert a deprecation warning for old function calls

[wholmgren]

I've decided that we should wait on this for an 0.6 release. It seems a little rude to make pvlib spit out a bunch of new warnings when people make a minor update to the package. The old functions would then be removed in the 0.7 release. We can adjust other issues to 0.7 or 0.8 get the changes out faster.

[wholmgren]

Need to look into this more detail, but maybe the pvlibDeprecationWarning should instead be a FutureWarning. FutureWarning is always shown, whereas DeprecationWarning is only sometimes shown (depends on Python version, though). See:

• https://docs.python.org/3/whatsnew/3.7.html#pep-565-show-deprecationwarning-in-main
• https://docs.python.org/3/library/warnings.html
• https://www.python.org/dev/peps/pep-0565/

[wholmgren]

@mikofski do you have suggestions/thoughts on deriving a pvlib warning from DeprecationWarning vs. FutureWarning? See comment above. It's not clear to me what the best approach is for us. Maybe FutureWarning?

[mikofski]

@wholmgren I hadn't realized the distinction between future and deprecation or pending deprecation warnings until now.

TL;DR:

According to my read of the PEP and Python Warnings docs, I think we should use FutureWarning.

more detail

Since PVLib is more like a developer toolbox, and less likely to be consumed by another application, it seems appropriate to issue verbose warnings. I think until Python-3.7 becomes more widespread, most users with Python-3.6 won't see deprecation or pending deprecation warnings unless they have turned warning filters off. If PEP-565 is backported to Python-3.6 then we can reconsider.

I also think we need guidelines in the contributor documentation that establish our procedures and some terms such as:

• how many releases should a FutureWarning be used before an item is deprecated?
• shall a deprecated feature be kept in a deprecated state forever, as a shim (see below), to maintain backwards compatibility?
• should a list of future and deprecated features be kept somewhere?
• what annotations are used in the documentation like .. versionadded: and .. deprecated: and are ad hoc warnings also used?
• when if ever is PendingDeprecationWarning used?

Pandas seems like a similar toolbox to PVLib, so I looked at it to see their MO. AFAICT: they seem to take the view that when new features will deprecate old ones, there are 3 stages:

1. both exist and are accepted (FutureWarning)
2. the feature is about to be deprecated (PendingDeprecationWarning)
3. the feature is deprecated, no longer in use and could be removed at any time (DeprecationWarning)

Pandas

I searched through the Pandas codebase and documents and here's my guess:

1. future warnings are used first, they indicate:

   • that in the future this command, argument, feature, api, syntax, etc. will change to something new
   • they are used when a new feature is added that would break backwards compatibility
   • sphinx documentation is annotated with .. versionadded:: x.y.z, for example see the join argument in concat
   • sphinx documentation is annotated with .. deprecated: x.y.z, for example see groupby.take convert negative indices flag
   • ad hoc warnings and .. warning: blah annotations are used in the documentation
   • after X releases, the feature is deprecated
   • items are listed in pandas issue 6581

2. PendingDeprecationWarning is used infrequently, perhaps because it is only used for 1 or 2 releases prior to actual deprecation

3. features that have actually been deprecated use deprecation warnings

   • sphinx documentation is annotated with .. deprecated:: x.y.z - there seems to be inconsistent guideline for when this should be added to the docs, but definitely by this state it must be present
   • warnings are used ad hoc if the feature may be removed in a future version
   • items are listed in pandas issue 13777
   • shims are left apparently indefinitely to maintain backwards compatibility, for example see the decorators.py shim that imports everything from _decorators

Pandas provides decorators to deprecated functions and kwargs which defaults to FutureWarning, and their contributor docs also say to issue a future warning.

[wholmgren]

Thank you for the detailed review @mikofski! This is very helpful.

Let's go with a warning inherited from FutureWarning in this PR. I will also consider adding .. versionadded:: x.y.z and .. deprecated: x.y.z to the docs where appropriate.

Let's discuss the other questions in a separate issue and documentation PR.

[wholmgren]

This PR has gone on for so long that I've forgotten where I started... It turns out that the deprecation code I copied from Matplotlib/Metpy uses UserWarning to avoid the DeprecationWarning issues discussed above. I could not find any discussion in Matplotlib about why UserWarning was chosen over FutureWarning. I have a mild preference for continuing to follow the Matplotlib pattern. If this is ok with others, then I think the PR is ready to merge.

[cwhanse]

At the risk of prolonging the PR unnecessarily, why not get_absolute_airmass here?

[wholmgren]

pvlib only includes one absolute airmass model so it didn't occur to me to add get_ here. I'm fine with either approach at this point.

[cwhanse]

I see your point. It may help when someone is trying to find the relevant function, to keep names parallel. There are other models for absolute air mass which we haven't implement.

[wholmgren]

Changed.

[wholmgren]

This PR probably makes a mess of the notebook tutorials and documentation. Probably best to fix them before merging.

[wholmgren]

I think this is finally ready to merge.

I updated the code in the sphinx documentation and the tutorials to use the new functions. Rendered documentation here and tutorials here. Minor related documentation changes... added plt.close() calls to avoid matplotlib memory warnings when building the sphinx documentation. Also removed most traces of seaborn from the documentation and tutorials. No need for seaborn in our documentation now that matplotlib has better default styles.

The appveyor/windows python 2.7 test configurations were failing on a test of a new deprecation test decorator. The same test passed on travis python 2.7 and earlier in this PR it also passed on appveyor (with nothing relevant to it changed since then). I modified the test to allow it to fail on windows on python 2.7.

[mikofski]

This is a massive overhaul, it LGTM, altho I had to get out my glasses bc I reviewed it on my phone

[wholmgren]

@mikofski thanks for reviewing this!

@cwhanse ready to merge or do you want to leave this open for more feedback? Go ahead and "squash and merge" when you think the time is right.