-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Changes from 3 commits
94259c5
862a28d
171e32e
c275a84
bbf5da4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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`. | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||
|
||||||||||||||||
|
||||||||||||||||
Bug fixes | ||||||||||||||||
~~~~~~~~~ | ||||||||||||||||
|
@@ -74,3 +78,4 @@ Contributors | |||||||||||||||
* Marcos R. Escudero (:ghuser:`marc-resc`) | ||||||||||||||||
* Bernat Nicolau (:ghuser:`BernatNicolau`) | ||||||||||||||||
* Eduardo Sarquis (:ghuser:`EduardoSarquis`) | ||||||||||||||||
* Andrew B Godbehere (:ghuser:`agodbehere`) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without the check the next line divides by |
||
# 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
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, thanks ❣️