-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Incidence Angle Modifier zero instead of np.nan (#338) #339
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
Changes from 4 commits
24d8fbc
acacf3a
6167d7f
d92f8fb
fbbff59
51340e2
990acf1
3d3e8b5
31461b1
28c5e17
23e67e1
a607a10
a329a8a
fd593a2
6ca99af
a52dba0
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 |
---|---|---|
|
@@ -709,7 +709,7 @@ def ashraeiam(aoi, b=0.05): | |
The incident angle modifier calculated as 1-b*(sec(aoi)-1) as | ||
described in [2,3]. | ||
|
||
Returns nan for all abs(aoi) >= 90 and for all IAM values that | ||
Returns zeros for all abs(aoi) >= 90 and for all IAM values that | ||
would be less than 0. | ||
|
||
References | ||
|
@@ -732,7 +732,7 @@ def ashraeiam(aoi, b=0.05): | |
|
||
iam = 1 - b*((1/np.cos(np.radians(aoi)) - 1)) | ||
|
||
iam = np.where(np.abs(aoi) >= 90, np.nan, iam) | ||
iam = np.where(np.abs(aoi) >= 90, 0, iam) | ||
iam = np.maximum(0, iam) | ||
|
||
if isinstance(iam, pd.Series): | ||
|
@@ -761,7 +761,8 @@ def physicaliam(aoi, n=1.526, K=4., L=0.002): | |
---------- | ||
aoi : numeric | ||
The angle of incidence between the module normal vector and the | ||
sun-beam vector in degrees. | ||
sun-beam vector in degrees. Angles of 0 are replaced with 1e-06 | ||
to ensure non-nan results. | ||
|
||
n : numeric | ||
The effective index of refraction (unitless). Reference [1] | ||
|
@@ -811,6 +812,10 @@ def physicaliam(aoi, n=1.526, K=4., L=0.002): | |
spa | ||
ashraeiam | ||
''' | ||
zeroang = 1e-06 | ||
|
||
aoi = np.where(aoi == 0, zeroang, aoi) | ||
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. Why did you add this line? I understand what it does, but not why we need it now when we didn't previously. It's not clear to me that we actually need zeroang at all. I think its only purpose is to avoid infs, but maybe those be more cleanly handled with np.where or similar. 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. As I understood it, if the input angle is 0, tau will be nan, which is why we substitute it with zeroang. I guess we could define tau0 as a constant in the function and handle aoi == 0 with np.where after the calculations are done, but I feel like the current function is more intuitive to understand. 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 should keep zeroang and for input theta < zeroang, physicaliam should return tau0. Passing theta=0 through will return nan rather than zero. Line 821 computes tau with the leading factor exp(-KL/cos(theta)) which is why we should return tau0 for small theta. tau0 is computed awkwardly. I'd replace line 829 - 833 with the limit value which we should have implemented in Matlab: 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. ok, now I see that you've changed the test so that an input of 0 returns 1 instead of np.nan. I should have caught this when I wrote the test. 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 am surprised that the model would not work for an angle of exactly zero degrees. Could this have something to do with the equation discrepancy mentioned in the comments further above? 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 don't think it has to do with the function discrepancy. The unmodified function for theta_r is not defined for angles > ~40°. The tau(theta)-function in turn simply has a discontinuity for theta = 0. It seems this was ignored in the paper... |
||
|
||
thetar_deg = tools.asind(1.0 / n*(tools.sind(aoi))) | ||
|
||
tau = (np.exp(- 1.0 * (K*L / tools.cosd(thetar_deg))) * | ||
|
@@ -819,8 +824,6 @@ def physicaliam(aoi, n=1.526, K=4., L=0.002): | |
((tools.tand(thetar_deg - aoi)) ** 2) / | ||
((tools.tand(thetar_deg + aoi)) ** 2)))))) | ||
|
||
zeroang = 1e-06 | ||
|
||
thetar_deg0 = tools.asind(1.0 / n*(tools.sind(zeroang))) | ||
|
||
tau0 = (np.exp(- 1.0 * (K*L / tools.cosd(thetar_deg0))) * | ||
|
@@ -831,7 +834,7 @@ def physicaliam(aoi, n=1.526, K=4., L=0.002): | |
|
||
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. Use 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 agree with your suggestions, especially after having consulted my D&B book. I would suggest calculating reflectance and absorption separately before combining them in order to clarify the code. 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 I applied both your suggestions correctly. Could you take a look? 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. It wasn't quite correct, so I thought it might be easier to communicate it this way:
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. Thanks for the detailed explanation! So does this refer to reference 2 in the Docstring (I don't have access so cannot 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. Ref [1] simply describes what is in ref [2]. 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 implemented your suggestion except for setting negative angles to np.nan, to keep consistency with the other IAM functions for now... 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. Consistency has some appeal... |
||
iam = tau / tau0 | ||
|
||
iam = np.where((np.abs(aoi) >= 90) | (iam < 0), np.nan, iam) | ||
iam = np.where((np.abs(aoi) >= 90) | (iam < 0), 0, iam) | ||
|
||
if isinstance(aoi, pd.Series): | ||
iam = pd.Series(iam, index=aoi.index) | ||
|
@@ -1462,7 +1465,7 @@ def sapm_aoi_loss(aoi, module, upper=None): | |
---------- | ||
aoi : numeric | ||
Angle of incidence in degrees. Negative input angles will return | ||
nan values. | ||
zeros. | ||
|
||
module : dict-like | ||
A dict, Series, or DataFrame defining the SAPM performance | ||
|
@@ -1504,7 +1507,7 @@ def sapm_aoi_loss(aoi, module, upper=None): | |
|
||
aoi_loss = np.polyval(aoi_coeff, aoi) | ||
aoi_loss = np.clip(aoi_loss, 0, upper) | ||
aoi_loss = np.where(aoi < 0, np.nan, aoi_loss) | ||
aoi_loss = np.where(aoi < 0, 0, aoi_loss) | ||
|
||
if isinstance(aoi, pd.Series): | ||
aoi_loss = pd.Series(aoi_loss, aoi.index) | ||
|
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.
is the np.maximum line still necessary?
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.
I think it is, because for angles close to +/-90° the function would return a negative value for the iam (the intercept for b=0.05 is at 87.21°: https://www.desmos.com/calculator/y8wvzrx3b9).
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.
I agree, the np.maximum line is needed