Skip to content

Convert irradiance.liujordan to irradiance.campbell_norman #1104

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.

Sign up for GitHub

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 10 commits into from
Dec 10, 2020
Merged

Convert irradiance.liujordan to irradiance.campbell_norman #1104

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fe486ec
initial campbellnormal
cwhanse Dec 8, 2020
afbd9e8
formatting, add deprecation test
cwhanse Dec 8, 2020
b2ba8a6
review
cwhanse Dec 9, 2020
19569a2
format
cwhanse Dec 9, 2020
5c9f95c
traceable deprecation
cwhanse Dec 9, 2020
bd54e7e
api.rst
cwhanse Dec 9, 2020
cc40e6a
change to campbell_normal in forecast.py
cwhanse Dec 9, 2020
d2229fd
remove stub for deprecation test in forecast.py
cwhanse Dec 9, 2020
02e2676
remove unused import
cwhanse Dec 9, 2020
999ecda
Update docs/sphinx/source/api.rst
cwhanse Dec 9, 2020
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
1 change: 1 addition & 0 deletions docs/sphinx/source/api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ DNI estimation models
irradiance.dirint
irradiance.dirindex
irradiance.erbs
irradiance.campbellnorman
irradiance.liujordan
irradiance.gti_dirint

Expand Down
6 changes: 5 additions & 1 deletion docs/sphinx/source/whatsnew/v0.8.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Breaking changes

Deprecations
~~~~~~~~~~~~

* :py:func:`pvlib.irradiance.liujordan` is deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


Enhancements
~~~~~~~~~~~~
Expand All @@ -21,6 +21,10 @@ Enhancements
multiple MPPTs (:issue:`457`, :pull:`1085`)
* Added optional ``attributes`` parameter to :py:func:`pvlib.iotools.get_psm3`
and added the option of fetching 5- and 15-minute PSM3 data. (:pull:`1086`)
* Added :py:func:`pvlib.irradiance.campbellnorman` for estimating DNI, DHI and GHI
from extraterrestrial irradiance. This function replaces :py:func:`pvlib.irradiance.liujordan`;
users of :py:func:`pvlib.irradiance.liujordan` should note that :py:func:`pvlib.irradiance.campbellnorman`
expects different parameters.

Bug fixes
~~~~~~~~~
Expand Down
59 changes: 59 additions & 0 deletions pvlib/irradiance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@

from pvlib import atmosphere, solarposition, tools

from pvlib._deprecation import deprecated


# see References section of grounddiffuse function
SURFACE_ALBEDOS = {'urban': 0.18,
'grass': 0.20,
Expand Down Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

Mild preference for campbell_norman.

'''
Determine DNI, DHI, GHI from extraterrestrial flux, transmittance,
and atmospheric pressure.

Parameters
----------
zenith: pd.Series
True (not refraction-corrected) zenith angles in decimal
degrees. If Z is a vector it must be of the same size as all
other vector inputs. Z must be >=0 and <=180.

transmittance: float
Atmospheric transmittance between 0 and 1.

pressure: float, default 101325.0
Air pressure

dni_extra: float, default 1367.0
Direct irradiance incident at the top of the atmosphere.

Returns
-------
irradiance: DataFrame
Modeled direct normal irradiance, direct horizontal irradiance,
and global horizontal irradiance in W/m^2

References
----------
.. [1] Campbell, G. S., J. M. Norman (1998) An Introduction to
Environmental Biophysics. 2nd Ed. New York: Springer.
'''

tau = transmittance

airmass = atmosphere.get_relative_airmass(zenith, model='simple')
airmass = atmosphere.get_absolute_airmass(airmass, pressure=pressure)
dni = dni_extra*tau**airmass
dhi = 0.3 * (1.0 - tau**airmass) * dni_extra * np.cos(np.radians(zenith))
ghi = dhi + dni * np.cos(np.radians(zenith))
Copy link
Member

Choose a reason for hiding this comment

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

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


irrads = OrderedDict()
irrads['ghi'] = ghi
irrads['dni'] = dni
irrads['dhi'] = dhi

if isinstance(ghi, pd.Series):
irrads = pd.DataFrame(irrads)

return irrads


def liujordan(zenith, transmittance, airmass, dni_extra=1367.0):
'''
Determine DNI, DHI, GHI from extraterrestrial flux, transmittance,
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

remove irradiance.liujordan from api.rst?

Copy link
Member

Choose a reason for hiding this comment

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

yes



def _get_perez_coefficients(perezmodel):
'''
Find coefficients for the Perez model
Expand Down
22 changes: 21 additions & 1 deletion pvlib/tests/test_irradiance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@

from pvlib import irradiance

from conftest import requires_ephem, requires_numba
from conftest import requires_ephem, requires_numba, fail_on_pvlib_version
from pvlib._deprecation import pvlibDeprecationWarning


# fixtures create realistic test input data
Expand Down Expand Up @@ -295,6 +296,17 @@ def test_liujordan():
assert_frame_equal(out, expected)
Copy link
Member

Choose a reason for hiding this comment

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

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

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 prefer the option after "or"



def test_campbellnorman():
expected = pd.DataFrame(np.array(
[[863.859736967, 653.123094076, 220.65905025]]),
columns=['ghi', 'dni', 'dhi'],
index=[0])
out = irradiance.campbellnorman(
pd.Series([10]), pd.Series([0.5]), pd.Series([109764.21013135818]),
dni_extra=1400)
assert_frame_equal(out, expected)


def test_get_total_irradiance(irrad_data, ephem_data, dni_et, relative_airmass):
models = ['isotropic', 'klucher',
'haydavies', 'reindl', 'king', 'perez']
Expand Down Expand Up @@ -928,3 +940,11 @@ def test_clearness_index_zenith_independent(airmass_kt):
airmass)
expected = pd.Series([np.nan, 0.553744437562], index=times)
assert_series_equal(out, expected)


@fail_on_pvlib_version('0.9')
def test_deprecated_09():
# deprecated function irradiance.liujordan
with pytest.warns(pvlibDeprecationWarning):
irradiance.liujordan(
pd.Series([10]), pd.Series([0.5]), pd.Series([1.1]), dni_extra=1400)