Detect clearsky modifications #510
Conversation
There was a problem hiding this comment.
I feel strongly (perhaps to the point of insisting) that this function provide a warning if passed data with timesteps different enough from 1 minute (say outside of 30s to 2 min) and the default thresholds values are used. The validation of this algorithm only used ~1 min timesteps, and the default threshold values derive from that validation.
| @@ -687,7 +687,7 @@ def detect_clearsky(measured, clearsky, times, window_length, | |||
| raise NotImplementedError('algorithm does not yet support unequal ' \ | |||
| 'times. consider resampling your data.') | |||
|
|
|||
| samples_per_window = int(window_length / sample_interval) | |||
| samples_per_window = int(window_length / sample_interval) + 1 | |||
There was a problem hiding this comment.
Revert this change please. samples_per_window is counting intervals, not endpoints. Because it's an intermediate it could be renamed to be more clear.
There was a problem hiding this comment.
I believe that not adding 1 gives the incorrect number of samples per window. For example, if my data is 30-minute frequency (sample_interval=30) and I want 60 minute windows (window_length=60), the current implementation would only give 2 points per window (when the Hankel matrix is constructed in the following lines). In this case, 2 points per window would only span a 30 minute window, not the intended 60.
There was a problem hiding this comment.
Your example makes my point. The algorithm operates on intervals not on points in time. The value at a timestamp is considered as the value for the following (?) interval - I'd have to look carefully at the Hankel matrix and the diff to see if we adopted a left- or right- endpoint convention.
| @@ -697,25 +697,27 @@ def detect_clearsky(measured, clearsky, times, window_length, | |||
| # 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) | |||
| meas_slope = np.diff(measured[H], n=1, axis=0) / sample_interval | |||
There was a problem hiding this comment.
The non-uniform time steps could be handled here by
- converting the time to UNIX timestamps
diffing the time to get time intervals- element-wise division in the calculation of
meas_slope - using the
diffed time in the calculation ofmeas_line_length, 'clear_slope`, etc.
This could be the subject of a subsequent pull request. In hindsight, I could have separated #507 into two issues (different from 1 minute, and non-uniform time steps.)
|
completed in #596 |
Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):
The fifth condition (
c5) of theclearsky.detect_clearskyfunction wasn't calculated as defined in the original reference (equations 12 - 14). This pull request also addresses part of #507 where there was an implicit assumption of minutely data. All derivatives are now calculated by explicitly dividing by thesample_interval. This does not address non-uniform time intervals.clearsky.detect_clearsky#506 (addresses part of Handle different time intervals in clearsky.detect_clearsky #507)docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfile for all changes.git diff upstream/master -u -- "*.py" | flake8 --diff