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

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

Open
pverspeelt opened this issue Sep 30, 2018 · 2 comments
Labels
question User questions and open questions about development

Comments

@pverspeelt
Copy link
Contributor

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
Copy link
Contributor

braverock commented Oct 1, 2018

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
Copy link
Contributor Author

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.

@joshuaulrich joshuaulrich added the question User questions and open questions about development label Oct 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question User questions and open questions about development
Projects
None yet
Development

No branches or pull requests

3 participants