Closed claymoremarshall closed 3 years ago
Thanks for the report! You are the first to identify and report this bug. Some background: this functionality used to be handled by .parseISO8601()
, but is now handled by .subsetTimeOfDay()
.
I just pushed a change to replace the indexTZ()
call with tzone()
, so that warning should go away. Thanks for catching that too.
It would be great if you could try to fix this, and even better if you add unit tests to cover this case. As you suggested in an email, it would make sense to add some intraday data to the package to use for examples and tests. Similar to how we use sample_matrix
.
I imagine there are going to be several edge cases we need to cover. So please don't hesitate to discuss them here before spending time coding a solution.
A couple of edge cases related to a "standard" time of day subset, e.g.
x["T02:00/T03:00"]
might be:
1) x["T0200/T0300"]
(currently fails because the as.POSIXct
conversion in timestringToSeconds
fails for "1970-01-01 0200"
).
Does the subsetting want to support this, or enforce users to do "T02:00/T03:00"?
2) Do you want to support full support for odd edge cases such as:
i) x["T02/T02"] to return all data for the second hour
ii) Just throwing it out there, but would support for x["T02"]
to return all data with 02:xx:xx
timestamps (which would be same as subsetting with .indexhour(x) == 2)? I'd be inclined not to permit this, but maybe there would be some value in it
In the case of just supplying the hours, would you now expect
x["T02/T03"]
to be equivalent to x[.indexhour(x) %in% c(2,3)
or x[.indexhour(x) ==2 | .indexhour(x) == 3 & .indexmin(x) == 0 & .indexsec(x) == 0]
. IIRC xts supported the former previously. If we use the former, we'll have to modify timeStringToSeconds
for the start and end supplied timepoints.
The recent time of day subsetting changes have generated some unexpected results.
Subsetting by just hour used to work, but now gives incorrect results, and also seems to still depend on
indexTZ
:But we do at least get expected results if including the minutes (but still have the
indexTZ
issue)Happy to work on a PR to fix this issue if it hasn't been identified yet?