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

Implemented closed-form solution in detect_clearsky #2217

Merged
merged 5 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
5 changes: 5 additions & 0 deletions docs/sphinx/source/whatsnew/v0.11.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ Enhancements
* Added function for calculating wind speed at different heights,
:py:func:`pvlib.atmosphere.windspeed_powerlaw`.
(:issue:`2118`, :pull:`2124`)
* Implemented closed-form solution for alpha in detect_clearsky, obviating
the call to scipy.optimize that was prone to runtime errors and minimizing
computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`.
Copy link
Contributor

@echedey-ls echedey-ls Sep 19, 2024

Choose a reason for hiding this comment

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

Suggested change
computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`.
computation, :py:func:`pvlib.clearsky.detect_clearsky`. (:discuss:`2171`, :issue:`2216`, :pull:`2217`)

Copy link
Member

@cwhanse cwhanse Sep 19, 2024

Choose a reason for hiding this comment

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

Should be :issue:2171, :issue:2216

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated, thanks ❣️

Copy link
Contributor

Choose a reason for hiding this comment

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

Link the function in the first line and then no need to repeat it here at the end as (I think?) we only do that if it is a new function being added

Suggested change
* Implemented closed-form solution for alpha in detect_clearsky, obviating
the call to scipy.optimize that was prone to runtime errors and minimizing
computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`.
* Implemented closed-form solution for alpha in
:py:func:`pvlib.clearsky.detect_clearsky`, obviating
the call to scipy.optimize that was prone to runtime errors and minimizing
computation. (:issue:`2171`, :issue:`2216`, :pull:`2217`).



Bug fixes
~~~~~~~~~
Expand Down Expand Up @@ -74,3 +78,4 @@ Contributors
* Marcos R. Escudero (:ghuser:`marc-resc`)
* Bernat Nicolau (:ghuser:`BernatNicolau`)
* Eduardo Sarquis (:ghuser:`EduardoSarquis`)
* Andrew B Godbehere (:ghuser:`agodbehere`)
25 changes: 5 additions & 20 deletions pvlib/clearsky.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import numpy as np
import pandas as pd
from scipy.optimize import minimize_scalar
from scipy.linalg import hankel
import h5py

Expand Down Expand Up @@ -874,25 +873,11 @@ def detect_clearsky(measured, clearsky, times=None, infer_limits=False,
clear_meas = meas[clear_samples]
clear_clear = clear[clear_samples]

def rmse(alpha):
return np.sqrt(np.mean((clear_meas - alpha*clear_clear)**2))

optimize_result = minimize_scalar(rmse)
if not optimize_result.success:
try:
message = "Optimizer exited unsuccessfully: " \
+ optimize_result.message
except AttributeError:
message = "Optimizer exited unsuccessfully: \
No message explaining the failure was returned. \
If you would like to see this message, please \
update your scipy version (try version 1.8.0 \
or beyond)."
raise RuntimeError(message)

else:
alpha = optimize_result.x

# Compute arg min of MSE between model and observations
C = (clear_clear**2).sum()
if not (pd.isna(C) or C == 0): # safety check
Copy link
Contributor

Choose a reason for hiding this comment

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

What would physically mean that this value is zero or NaN? Maybe we can skip this safety check, I wonder what could happen if this code isn't executed

Copy link
Member

Choose a reason for hiding this comment

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

Without the check the next line divides by C. The exit criteria need alpha to be a float. I think the check and if...break are fine.

# only update alpha if C is strictly positive
alpha = (clear_meas * clear_clear).sum() / C
if round(alpha*10000) == round(previous_alpha*10000):
break
else:
cwhanse marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
13 changes: 9 additions & 4 deletions pvlib/tests/test_clearsky.py
Original file line number Diff line number Diff line change
Expand Up @@ -675,16 +675,21 @@
def test_detect_clearsky_not_enough_data(detect_clearsky_data):
expected, cs = detect_clearsky_data
with pytest.raises(ValueError, match='have at least'):
clearsky.detect_clearsky(expected['GHI'], cs['ghi'], window_length=60)

Check failure on line 678 in pvlib/tests/test_clearsky.py

View workflow job for this annotation

GitHub Actions / flake8-linter

E111 indentation is not a multiple of 4


def test_detect_clearsky_optimizer_failed(detect_clearsky_data):
@pytest.mark.parametrize("window_length", [5, 10, 15, 20, 25])
def test_detect_clearsky_optimizer_not_failed(
detect_clearsky_data,

Check failure on line 683 in pvlib/tests/test_clearsky.py

View workflow job for this annotation

GitHub Actions / flake8-linter

W291 trailing whitespace
window_length
):

Check failure on line 685 in pvlib/tests/test_clearsky.py

View workflow job for this annotation

GitHub Actions / flake8-linter

E121 continuation line under-indented for hanging indent

Check failure on line 685 in pvlib/tests/test_clearsky.py

View workflow job for this annotation

GitHub Actions / flake8-linter

E125 continuation line with same indent as next logical line
expected, cs = detect_clearsky_data
with pytest.raises(RuntimeError, match='Optimizer exited unsuccessfully'):
clearsky.detect_clearsky(expected['GHI'], cs['ghi'], window_length=15)

clear_samples = clearsky.detect_clearsky(
expected["GHI"], cs["ghi"], window_length=window_length
)
assert isinstance(clear_samples, pd.Series)

@pytest.fixture

Check failure on line 692 in pvlib/tests/test_clearsky.py

View workflow job for this annotation

GitHub Actions / flake8-linter

E302 expected 2 blank lines, found 1
def detect_clearsky_helper_data():
samples_per_window = 3
sample_interval = 1
Expand Down
Loading