Skip to content
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

Correct usage of xtol in ghi_from_poa_driesse_2023 #1971

Merged
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
2 changes: 2 additions & 0 deletions docs/sphinx/source/whatsnew/v0.10.4.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ Bug fixes
~~~~~~~~~
* :py:class:`~pvlib.modelchain.ModelChain` now raises a more useful error when
``temperature_model_parameters`` are specified on the passed ``system`` instead of on its ``arrays``. (:issue:`1759`).
* :py:func:`pvlib.irradiance.ghi_from_poa_driesse_2023` now correctly makes use
of the ``xtol`` argument. Previously, it was ignored. (:issue:`1970`, :pull:`1971`)

Testing
~~~~~~~
Expand Down
9 changes: 6 additions & 3 deletions pvlib/irradiance.py
Original file line number Diff line number Diff line change
Expand Up @@ -1557,8 +1557,8 @@ def ghi_from_poa_driesse_2023(surface_tilt, surface_azimuth,
albedo : numeric, default 0.25
Ground surface albedo. [unitless]
xtol : numeric, default 0.01
Convergence criterion. The estimated GHI will be within xtol of the
true value. [W/m^2]
Convergence criterion. The estimated GHI will be within xtol of the
true value. Must be positive. [W/m^2]
Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, the bisect docstring says nonnegative, which is inconsistent with its code.

full_output : boolean, default False
If full_output is False, only ghi is returned, otherwise the return
value is (ghi, converged, niter). (see Returns section for details).
Expand Down Expand Up @@ -1593,13 +1593,16 @@ def ghi_from_poa_driesse_2023(surface_tilt, surface_azimuth,
'''
# Contributed by Anton Driesse (@adriesse), PV Performance Labs. Nov., 2023

if xtol <= 0:
raise ValueError(f"xtol too small ({xtol:g} <= 0)")

ghi_from_poa_array = np.vectorize(_ghi_from_poa)

ghi, conv, niter = ghi_from_poa_array(surface_tilt, surface_azimuth,
solar_zenith, solar_azimuth,
poa_global,
dni_extra, airmass, albedo,
xtol=0.01)
xtol=xtol)
Copy link
Member

Choose a reason for hiding this comment

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

Good catch!


if isinstance(poa_global, pd.Series):
ghi = pd.Series(ghi, poa_global.index)
Expand Down
18 changes: 17 additions & 1 deletion pvlib/tests/test_irradiance.py
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,7 @@ def test_dirint_min_cos_zenith_max_zenith():
assert_series_equal(out, expected, check_less_precise=True)


def test_ghi_from_poa_driesse():
def test_ghi_from_poa_driesse(mocker):
# inputs copied from test_gti_dirint
times = pd.DatetimeIndex(
['2014-06-24T06-0700', '2014-06-24T09-0700', '2014-06-24T12-0700'])
Expand Down Expand Up @@ -825,6 +825,22 @@ def test_ghi_from_poa_driesse():
expected = [0, -1, 0]
assert_allclose(expected, niter)

# test xtol argument
poa_global = pd.Series([20, 300, 1000], index=times)
# test exception
xtol = -3.14159 # negative value raises exception in scipy.optimize.bisect
with pytest.raises(ValueError, match=rf"xtol too small \({xtol:g} <= 0\)"):
output = irradiance.ghi_from_poa_driesse_2023(
surface_tilt, surface_azimuth, zenith, azimuth,
poa_global, dni_extra=1366.1, xtol=xtol)
# test propagation
xtol = 3.141592
bisect_spy = mocker.spy(irradiance, "bisect")
output = irradiance.ghi_from_poa_driesse_2023(
surface_tilt, surface_azimuth, zenith, azimuth,
poa_global, dni_extra=1366.1, xtol=xtol)
assert bisect_spy.call_args[1]["xtol"] == xtol


def test_gti_dirint():
times = pd.DatetimeIndex(
Expand Down
Loading