joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
220 stars 71 forks source link

Question: is there a reason why align.time doesn't accept a negative value for n? #274

Open pverspeelt opened 5 years ago

pverspeelt commented 5 years ago

Description

Is there a reason why align.time doesn't accept a negative value of n? There is a stop (if(n <= 0) stop("'n' must be positive")) programmed into the function to prevent this.

See this SO post for more background

Expected behavior

Instead of only rounding up with align.time, I expected to be able to round down to the nearest minute. I tested with my own versions of align.time without the stop in them and I haven't found an issue yet.

Minimal, reproducible example

x <- Sys.time() + 1:10
align.time(x, n = -60)

returns:

Error in align.time.POSIXct(x, n = -60) : 'n' must be positive

Session Info

R version 3.5.0 (2018-04-23)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows >= 8 x64 (build 9200)

Matrix products: default

locale:
[1] LC_COLLATE=Dutch_Netherlands.1252  LC_CTYPE=Dutch_Netherlands.1252    LC_MONETARY=Dutch_Netherlands.1252
[4] LC_NUMERIC=C                       LC_TIME=Dutch_Netherlands.1252    

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] xts_0.11-1 zoo_1.8-4 

loaded via a namespace (and not attached):
[1] compiler_3.5.0  tools_3.5.0     yaml_2.2.0      grid_3.5.0      lattice_0.20-35
braverock commented 5 years ago

The reason align.time only allows you to round up is that it would introduce look-ahead bias (data snooping) to round future observations down.

Any analysis that you did after rounding down would have observations that had in the original data taken place in the future from their new (aligned) index time. This can't happen in reality (you can't know the future), so we didn't want to support unwittingly bad analysis.

I suppose that there is no real reason it couldn't be supported with a warning(), but it still makes me uncomfortable that it would be (likely regularly) misused to introduce biases into data.

pverspeelt commented 5 years ago

I can see where you are coming from if we consider measurement or stock data. But xts is not just built for stock data. The use cases for rounding down are slim anyway. But in this case, maybe the documentation of align.time should be adjusted a bit to make it more clear.

The question that follows is whether the shift.time should be adjusted to prevent this as well. Because now I can use align.time to round to the next 5 minutes and then use shift.time to go back x minutes and circumvent the restriction which is built in align.time.

Also shift.time is only available for xts series and not Posixct. If you want I can create a PR request to add this to the code. And unit tests for align.time and shift.time. At the moment there are no unit tests for align.time and shift.time.