Correct condition 5 in detect_clearksy #596
Conversation
![stickler-ci[bot]](https://avatars.githubusercontent.com/in/15299?s=60&v=4){kind=small}
|
Ready for merge. I did not change the tests. The condition with the error (condition 5) is not the unique determining factor for clear or not clear for the test we have. Perhaps we should create a set of tests that fail at different times for each condition, one at a time. |
That would be ideal. I prefer making that change as part of this PR to ensure that it's properly tested. |
pvlib/clearsky.py
Outdated
| components['max_diff_array'] = np.abs(meas_max - alpha * clear_max) | ||
| components['line_length_array'] = meas_line_length - clear_line_length | ||
| components['slope_nstd_array'] = meas_slope_nstd | ||
| components['slope_max_array'] = (np.max( |
There was a problem hiding this comment.
maybe "values" instead of "array"?
There was a problem hiding this comment.
_flag on the previous set of keys, removed _array here
There was a problem hiding this comment.
I like the change, but it would break user code if people are calling return_components. Probably not likely to be a serious issue but we should think twice about doing this. In any case it should be listed as an API change.
There was a problem hiding this comment.
what was meant by "values" instead of "array"? I'm ok reverting this change, but I thought you were requesting renaming the keys.
There was a problem hiding this comment.
Sorry for not being clear. I originally meant to suggest only slightly different names for the new keys that you've added. I was not suggesting renaming the existing keys. I thought "array" was an undesirable identifier because everything is an array here.
I do like the latest proposal, but we'd need to document it as an API change.
pvlib/clearsky.py
Outdated
| clear_slope_max = np.max(np.abs(clear_slope), axis=0) | ||
| clear_ghi_diff = np.diff(clearsky[H], n=1, axis=0) | ||
| clear_slope = np.diff(clearsky[H], n=1, axis=0) / sample_interval | ||
| # clear_slope_max = np.max(np.abs(clear_slope), axis=0) |
pvlib/clearsky.py
Outdated
| # matlab std function normalizes by N-1, so set ddof=1 here | ||
| meas_slope_nstd = np.std(meas_slope, axis=0, ddof=1) / meas_mean | ||
| meas_slope_max = np.max(np.abs(meas_slope), axis=0) | ||
| # meas_slope_max = np.max(np.abs(meas_slope), axis=0) |
pvlib/clearsky.py
Outdated
|
|
||
| # calculate measurement statistics | ||
| meas_mean = np.mean(measured[H], axis=0) | ||
| meas_max = np.max(measured[H], axis=0) | ||
| meas_slope = np.diff(measured[H], n=1, axis=0) | ||
| meas_ghi_diff = np.diff(measured[H], n=1, axis=0) |
There was a problem hiding this comment.
I prefer meas_diff to meas_ghi_diff. None of the other code in this function specifically refers to ghi.
pvlib/clearsky.py
Outdated
|
|
||
| # calculate clear sky statistics | ||
| clear_mean = np.mean(clearsky[H], axis=0) | ||
| clear_max = np.max(clearsky[H], axis=0) | ||
| clear_slope = np.diff(clearsky[H], n=1, axis=0) | ||
| clear_slope_max = np.max(np.abs(clear_slope), axis=0) | ||
| clear_ghi_diff = np.diff(clearsky[H], n=1, axis=0) |
There was a problem hiding this comment.
same comment as above re "ghi"
|
Thanks for advancing this @cwhanse and @wholmgren! I believe this will also close #507, depending on whether #507 is interpreted to include the handling of non-uniform time intervals or not. |
|
We can ignore the failing 2.7-min configuration as it's transitioned to pandas 0.15 in a few PRs. |
It is turning out to be rather challenging too cook up data that cause only one condition to fail but not others. How much do we care here about being less than ideal? |
I don't care enough to hold up this PR. Thanks for trying. |
|
ready I think. cause of py27 failures appear to be addressed in #583 |
pvlib python pull request guidelines
Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.
You may submit a pull request with your code at any stage of completion.
The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:
clearsky.detect_clearsky#506docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfile for all changes.Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
Imports changes from @bhellis #510.