Skip to content
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

Handle different time intervals in clearsky.detect_clearsky #507

Open
cwhanse opened this issue Jul 18, 2018 · 6 comments
Open

Handle different time intervals in clearsky.detect_clearsky #507

cwhanse opened this issue Jul 18, 2018 · 6 comments
Assignees
Milestone

Comments

@cwhanse
Copy link
Member

cwhanse commented Jul 18, 2018

Is your feature request related to a problem? Please describe.
clearsky.detect_clearsky implicitly assumes data at 1 minute time intervals. See #505

Describe the solution you'd like
clearsky.detect_clearsky should be capable of accepting data at different, and possibly non-uniform, time intervals. PVLib MATLAB function pvl_detect_clear_times provides an example method.

@wholmgren
Copy link
Member

Wouldn't it be relatively straightforward to accept uniform intervals of any length by multiplying the supplied coefficients by a scaling factor?

I'm guessing that the non-uniform case should be implemented in a separate function that's called within detect_clearsky. Probably move the uniform case to a separate function, too. Possibly private functions for simplicity, but perhaps there is value in exposing them to the public api.

@adriesse
Copy link
Member

I guess we never finished the discussion in #433, where I cited this function as an example where a warning might be useful. Or perhaps more accurately, I never acted on the implementation suggestion!

If the model was developed, tested and published for and with 1-minute data, then modifying it for other sampling rates should get additional validation.

@cwhanse
Copy link
Member Author

cwhanse commented Jul 18, 2018

@wholmgren up to a point, yes, but the threshold criteria should be re-considered if one is applying this method to data at much longer intervals.

@adriesse good point. The paper looked only at 1 minute data. I've used versions of that algorithm on roughly one minute data (30s to 90s, not uniform intervals) and got reasonable results. At 15 minute intervals or longer I'd question the outcome. I'd be OK with a gentle warning about validation of the algorithm. I'd be more than OK if someone wants to actually validate the algorithm, or show how it should change, to handle longer time intervals.

@AndyMender
Copy link

I think it's alright to allow intervals other than 1 minute provided that the data points fit within the span of a single day (1440 minutes). Unevenly spaced data might be a major issue for the entire algorithm because of the static window size.

@mdeceglie
Copy link
Contributor

I suggest we plan to treat two issues/enhancements separately:

  1. Eliminate the implicit assumption of 1 minute data
  2. Allow for non-uniform timestamps.

Item 1 is relatively straight forward to address, and I think @bhellis725 has already addressed it here: https://github.com/bhellis725/pvlib-python/tree/clearsky Hopefully we'll see a pull request on that soon. Item 2 would be a great addition, but is a heavier lift, so I'd be in favor of at least advancing item 1 in the near term.

@benbenboben
Copy link

I just submitted a pull request that addresses some of these concerns and the concerns in #506 (I made both fixes simultaneously, so they're in the same PR).

In #506, there is mention of modifying the test for the detect_clearsky function. I'm not sure how to best proceed in modifying this test or how the test was originally conceived, so I'd like some input on how to handle that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants