-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Replace interp1d #2394 #2741
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
base: main
Are you sure you want to change the base?
Replace interp1d #2394 #2741
Changes from all commits
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |||||
| import functools | ||||||
| from scipy.optimize import minimize | ||||||
| from pvlib.tools import cosd, sind, acosd | ||||||
| from scipy.interpolate import make_interp_spline | ||||||
|
|
||||||
| # a dict of required parameter names for each IAM model | ||||||
| # keys are the function names for the IAM models | ||||||
|
|
@@ -440,7 +441,7 @@ def interp(aoi, theta_ref, iam_ref, method='linear', normalize=True): | |||||
| method : str, default 'linear' | ||||||
| Specifies the interpolation method. | ||||||
| Useful options are: 'linear', 'quadratic', 'cubic'. | ||||||
| See scipy.interpolate.interp1d for more options. | ||||||
| See scipy.interpolate for more options. | ||||||
|
Member
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. This line may need to be edited, depending on https://github.com/pvlib/pvlib-python/pull/2741/changes#r3137773820 |
||||||
|
|
||||||
| normalize : boolean, default True | ||||||
| When true, the interpolated values are divided by the interpolated | ||||||
|
|
@@ -469,9 +470,10 @@ def interp(aoi, theta_ref, iam_ref, method='linear', normalize=True): | |||||
| pvlib.iam.sapm | ||||||
| ''' | ||||||
| # Contributed by Anton Driesse (@adriesse), PV Performance Labs. July, 2019 | ||||||
|
|
||||||
| from scipy.interpolate import interp1d | ||||||
|
|
||||||
| if isinstance(theta_ref, list): | ||||||
|
Member
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. We normally don't check the type of input. If a user inputs a non-allowed type e.g. a list, |
||||||
| raise TypeError("theta_ref cannot be a list") | ||||||
| if isinstance(iam_ref, list): | ||||||
| raise TypeError("iam_ref cannot be a list") | ||||||
| # Scipy doesn't give the clearest feedback, so check number of points here. | ||||||
| MIN_REF_VALS = {'linear': 2, 'quadratic': 3, 'cubic': 4, 1: 2, 2: 3, 3: 4} | ||||||
|
Member
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. You could add a dictionary with names and k values, or something like that, to use below. |
||||||
|
|
||||||
|
|
@@ -483,10 +485,19 @@ def interp(aoi, theta_ref, iam_ref, method='linear', normalize=True): | |||||
| raise ValueError("Negative value(s) found in 'iam_ref'. " | ||||||
| "This is not physically possible.") | ||||||
|
|
||||||
| interpolator = interp1d(theta_ref, iam_ref, kind=method, | ||||||
| fill_value='extrapolate') | ||||||
| aoi_input = aoi | ||||||
| if method == "linear": | ||||||
| interpolator = make_interp_spline(theta_ref, iam_ref, k=1) | ||||||
|
|
||||||
| elif method == "quadratic": | ||||||
| interpolator = make_interp_spline(theta_ref, iam_ref, k=2) | ||||||
|
|
||||||
| elif method == "cubic": | ||||||
| interpolator = make_interp_spline(theta_ref, iam_ref, k=3) | ||||||
|
|
||||||
| else: | ||||||
| raise ValueError(f"Invalid interpolation method '{method}'.") | ||||||
|
Member
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. This is, technically, a breaking change, since the
Contributor
Author
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. I'm not sure I know how to update to support all of those.
Member
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. Since
Member
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. I think that's reasonable, as long as we deprecate these weird
Member
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. None of those options really make any sense here. I would vote for dropping them immediately.
Member
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.
Suggested change
|
||||||
|
|
||||||
| aoi_input = aoi | ||||||
| aoi = np.asanyarray(aoi) | ||||||
| aoi = np.abs(aoi) | ||||||
| iam = interpolator(aoi) | ||||||
|
|
||||||
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.
This is one where performance is likely important. Maybe check if np.interp is faster?