Convert irradiance.liujordan to irradiance.campbell_norman #1104

cwhanse · 2020-12-08T21:06:18Z

Closes Docstring for irradiance.liujordan isn't clear #1100
I am familiar with the contributing guidelines
Tests added
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Replaces irradiance.liujordan with irradiance.campbellnorman. liujordan is deprecated.

wholmgren

also need to deprecate

ForecastModel.cloud_cover_to_irradiance_liujordan and replace with ForecastModel.cloud_cover_to_irradiance_campbell_norman
the 'liujordan' option in ForecastModel.cloud_cover_to_irradiance.

wholmgren · 2020-12-08T22:24:16Z

pvlib/irradiance.py

@@ -2184,6 +2187,58 @@ def erbs(ghi, zenith, datetime_or_doy, min_cos_zenith=0.065, max_zenith=87):
    return data


+def campbellnorman(zenith, transmittance, pressure=101325.0, dni_extra=1367.0):


Mild preference for campbell_norman.

wholmgren · 2020-12-08T22:25:28Z

pvlib/irradiance.py

+    dhi = 0.3 * (1.0 - tau**airmass) * dni_extra * np.cos(np.radians(zenith))
+    ghi = dhi + dni * np.cos(np.radians(zenith))


Suggested change

dhi = 0.3 * (1.0 - tau**airmass) * dni_extra * np.cos(np.radians(zenith))

ghi = dhi + dni * np.cos(np.radians(zenith))

cos_zen = np.cos(np.radians(zenith))

dhi = 0.3 * (1.0 - tau**airmass) * dni_extra * cos_zen

ghi = dhi + dni * cos_zen

wholmgren · 2020-12-08T22:30:13Z

pvlib/tests/test_irradiance.py

@@ -295,6 +296,17 @@ def test_liujordan():
    assert_frame_equal(out, expected)


This test is going to emit a deprecation warning and we want to avoid that. We could delete it or we could skip the new test_deprecated_09 test, add @fail_on_pvlib_version('0.9') around this test, and add the with pytest.warns... here

I prefer the option after "or"

wholmgren · 2020-12-08T22:32:02Z

pvlib/irradiance.py

@@ -2242,6 +2297,10 @@ def liujordan(zenith, transmittance, airmass, dni_extra=1367.0):
    return irrads


+liujordan = deprecated('0.8', alternative='campbellnormam',
+                       name='liujordan', removal='0.9')(liujordan)


I don't think there's anything wrong with this, but it might be a little more clear if we change the function definition to _liujordan so that readers of the code cannot miss the deprecation wrapper.

Not needed if the deprecation warning and @fail_on_pvlib_version('0.9') are made part of the old test_liujordan, right?

My concern is just that we're redefining what liujordan points to (first it points to the original function, then we redefine it to point to a function that wraps the original function). There's no practical difference for a user of the public API, but a reader of the source code could miss the redefinition of liujordan.

def _liujordan(...) # the original docstring and code liujordan = deprecated(...)(_liujordan)

It's minor and I'm fine to leave it as is.

The tests as written in the latest commit (b2ba8a6) should continue to work without modification.

remove irradiance.liujordan from api.rst?

wholmgren · 2020-12-08T22:34:07Z

docs/sphinx/source/whatsnew/v0.8.1.rst

@@ -9,7 +9,7 @@ Breaking changes

 Deprecations
 ~~~~~~~~~~~~
-
+* :py:func:`pvlib.irradiance.liujordan` is deprecated.


these links will break and cause documentation build warnings when we remove the function, so I'd rather use double ticks now

wholmgren · 2020-12-08T22:34:20Z

docs/sphinx/source/whatsnew/v0.8.1.rst

@@ -9,7 +9,7 @@ Breaking changes

 Deprecations
 ~~~~~~~~~~~~
-
+* :py:func:`pvlib.irradiance.liujordan` is deprecated.


Suggested change

* :py:func:`pvlib.irradiance.liujordan` is deprecated.

* ``pvlib.irradiance.liujordan`` is deprecated.

cwhanse · 2020-12-09T18:43:11Z

test failures related to get_psm3

cwhanse · 2020-12-09T21:37:28Z

@wholmgren do you think it is worthwhile / necessary to add tests for forecast.cloud_cover_to_irradiance_how for how='campbell_norman' and 'liujordan'? The latter would only be a deprecation test. It looks like it would take a bit of work to assemble the test input.

wholmgren · 2020-12-09T21:56:34Z

not necessary

cwhanse · 2020-12-09T22:14:59Z

test failures are get_psm3. coverage decreases because we're not testing the deprecated forecast method. I didn't remove the deprecated method.

docs/sphinx/source/api.rst

correct misspelling Co-authored-by: Will Holmgren <[email protected]>

kandersolar · 2020-12-15T04:55:39Z

FYI looks like this introduced a docs build warning in forecast.rst:

>>>-------------------------------------------------------------------------
Warning in /home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/latest/docs/sphinx/source/forecasts.rst at block ending on line 281
Specify :okwarning: as an option in the ipython:: block to suppress this message
----------------------------------------------------------------------------
/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/latest/pvlib/forecast.py:637: pvlibDeprecationWarning: The Forecast.cloud_cover_to_irradiance_liujordan function was deprecated in pvlib 0.8 and will be removed in 0.9. Use Forecast.cloud_cover_to_irradiance_campbell_norman instead.
  cloud_cover, **kwargs)
<<<-------------------------------------------------------------------------

cwhanse added 2 commits December 8, 2020 14:04

initial campbellnormal

fe486ec

formatting, add deprecation test

afbd9e8

wholmgren reviewed Dec 8, 2020

View reviewed changes

cwhanse added 2 commits December 9, 2020 09:22

review

b2ba8a6

format

19569a2

cwhanse added this to the 0.8.1 milestone Dec 9, 2020

cwhanse added api documentation labels Dec 9, 2020

traceable deprecation

5c9f95c

cwhanse added 3 commits December 9, 2020 11:55

api.rst

bd54e7e

change to campbell_normal in forecast.py

cc40e6a

remove stub for deprecation test in forecast.py

d2229fd

remove unused import

02e2676

wholmgren reviewed Dec 9, 2020

View reviewed changes

docs/sphinx/source/api.rst Outdated Show resolved Hide resolved

Update docs/sphinx/source/api.rst

999ecda

correct misspelling Co-authored-by: Will Holmgren <[email protected]>

wholmgren changed the title ~~Convert irradiance.liujordan to irradiance.campbellnorman~~ Convert irradiance.liujordan to irradiance.campbell_norman Dec 10, 2020

wholmgren approved these changes Dec 10, 2020

View reviewed changes

wholmgren merged commit 74df8b6 into pvlib:master Dec 10, 2020

cwhanse deleted the liu branch December 11, 2020 15:13

wholmgren mentioned this pull request Dec 16, 2020

replace liu jordan with campbell norman in forecast.rst #1114

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert irradiance.liujordan to irradiance.campbell_norman #1104

Convert irradiance.liujordan to irradiance.campbell_norman #1104

cwhanse commented Dec 8, 2020 •

edited

Loading

wholmgren left a comment

wholmgren Dec 8, 2020

wholmgren Dec 8, 2020

wholmgren Dec 8, 2020

cwhanse Dec 9, 2020

wholmgren Dec 8, 2020

cwhanse Dec 9, 2020

wholmgren Dec 9, 2020

cwhanse Dec 9, 2020

wholmgren Dec 9, 2020

wholmgren Dec 8, 2020

wholmgren Dec 8, 2020

cwhanse commented Dec 9, 2020

cwhanse commented Dec 9, 2020 •

edited

Loading

wholmgren commented Dec 9, 2020

cwhanse commented Dec 9, 2020

kandersolar commented Dec 15, 2020

		@@ -2184,6 +2187,58 @@ def erbs(ghi, zenith, datetime_or_doy, min_cos_zenith=0.065, max_zenith=87):
		return data


		def campbellnorman(zenith, transmittance, pressure=101325.0, dni_extra=1367.0):

		dhi = 0.3 * (1.0 - tau*airmass) dni_extra * np.cos(np.radians(zenith))
		ghi = dhi + dni * np.cos(np.radians(zenith))

-    dhi = 0.3 * (1.0 - tau**airmass) * dni_extra * np.cos(np.radians(zenith))
-    ghi = dhi + dni * np.cos(np.radians(zenith))
+    cos_zen = np.cos(np.radians(zenith))
+    dhi = 0.3 * (1.0 - tau**airmass) * dni_extra * cos_zen
+    ghi = dhi + dni * cos_zen

		@@ -295,6 +296,17 @@ def test_liujordan():
		assert_frame_equal(out, expected)

	* :py:func:`pvlib.irradiance.liujordan` is deprecated.
	* ``pvlib.irradiance.liujordan`` is deprecated.

Convert irradiance.liujordan to irradiance.campbell_norman #1104

Convert irradiance.liujordan to irradiance.campbell_norman #1104

Conversation

cwhanse commented Dec 8, 2020 • edited Loading

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Dec 9, 2020

cwhanse commented Dec 9, 2020 • edited Loading

wholmgren commented Dec 9, 2020

cwhanse commented Dec 9, 2020

kandersolar commented Dec 15, 2020

cwhanse commented Dec 8, 2020 •

edited

Loading

cwhanse commented Dec 9, 2020 •

edited

Loading