Refactor detect_clearsky #1074
Refactor detect_clearsky #1074
Conversation
|
if I need to add some tests for the non-trivial helper functions, the We could drop |
|
Is there a compelling use case for I will carefully review when I have the time. I want to understand why this implementation works but the previous attempts to use pandas did not work. |
|
I also think we could drop support for |
|
To put it differently: why should we preserve the old behavior? It sounds like a bug. People that want the old behavior can use an old, buggier version of pvlib.
agree |
|
I guess I'm unwilling to call the left alignment a bug - it's not wrong, rather, it's an undesirable behavior. I'm OK changing the default to 'center'. And open to not offering 'left', 'right' as options. |
|
Other than consistency with previous versions, when might a user prefer 'left' or 'right'? Without a compelling reason, I'd hard-code center and then let users ask/contribute if they come up with a good reason for a kwarg. |
Consistency is the only reason I came up with. The option to align 'left', 'center', 'right' falls out of the combination of numpy and pandas defaults.
I'm OK with this. Unless you object I'll keep the 'align' plumbing in the private functions. |
…etect_clearsky
…etect_clearsky
|
@wholmgren this looks better on the profiler |
I'm willing to accept this performance decrease if we think it's a net win for readability and ease of reuse in future algorithms. |
|
@wholmgren I removed the use of |
There was a problem hiding this comment.
asv compare pvlib/master cwhanse/detect_clearsky
All benchmarks:
before after ratio
[926d2f95] [e53572d9]
<detect_clearsky>
+ 14.7±0.4ms 54.0±0.5ms 3.66 detect_clearsky.DetectClear.time_detect_clearsky [holmgren-mbp.local/conda-py3.6-ephem3.7.6.0-numba0.36.1-numpy1.12.0-pandas0.22.0-pytables3.6.1-scipy1.2.0]
+ 15.1±0.1ms 52.4±1ms 3.47 detect_clearsky.DetectClear.time_detect_clearsky [holmgren-mbp.local/conda-py3.8-ephem-numba-numpy-pandas-pytables-scipy]
|
@cwhanse I took the liberty of parameterizing the test in the latest commit, so you'll want to pull before you commit again. Results below with The performance penalty is not so bad for longer data sets. |
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
Yes for me. @kanderso-nrel ? |
| @@ -569,19 +579,19 @@ def test_detect_clearsky_iterations(detect_clearsky_data): | |||
| alpha = 1.0448 | |||
| with pytest.warns(RuntimeWarning): | |||
| clear_samples = clearsky.detect_clearsky( | |||
| expected['GHI'], cs['ghi']*alpha, cs.index, 10, max_iterations=1) | |||
| assert (clear_samples[:'2012-04-01 10:41:00'] == True).all() | |||
| assert (clear_samples['2012-04-01 10:42:00':] == False).all() | |||
There was a problem hiding this comment.
De Morgan wonders if this should use .any() instead of .all()?
There was a problem hiding this comment.
Oops, this comment is about the line assert not clear_samples['2012-04-01 10:42:00':].all() # expected False
There was a problem hiding this comment.
You are correct, should be .any()
docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).Refactor of
clearsky.detect_clearskyin preparation for adding BrightSun detect clearsky algorithm. Major changes:align(default 'left') otherwise function signature is unchanged.