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

lines 34-38, edited; incorrect if statement #289

Closed ReesaJohn closed 1 year ago

ReesaJohn commented 5 years ago

I think that if statement prevents anything timeBased from using Periodicity

joshuaulrich commented 5 years ago

Thanks for the report. Do you have a reproducible example of the behavior you think is incorrect? I'm reluctant to change code without concrete examples of incorrect behavior.

I can't verify your hypothesis that the current code doesn't allow periodicity() to work on time-based (e.g. Date, POSIXt, chron, timeDate, yearmon, yearqtr, etc):

xts::periodicity(Sys.Date()-10:1)
# Daily periodicity from 2019-03-04 to 2019-03-13
xts::periodicity(Sys.time()-runif(10, 0, 86400))
# Hourly periodicity from 2019-03-13 20:06:35 to 2019-03-14 13:49:12 
joshuaulrich commented 5 years ago

@ReesaJohn can you please respond to the questions in my previous comment? I would like to fix the issue, but I need more evidence of the actual problem. That way, I can be sure we're solving the root cause, and not a downstream symptom.

ReesaJohn commented 5 years ago

Hi Joshua, I found the error when I tried using the anomalize package's time_decompose method on a tibbletime object with the date column formatted with as.POSIXct.Date().

The error I got was "error in try.xts(x, error = "'x' needs to be timeBased or xtsible") : 'x' needs to be timeBased or xtsible".

I do not get this issue however with R data sets like FANG. When I saw your comment, I assumed I may have formatted my data wrong in some way, since I was able to get it to work with FANG.

That said the issue still persists.

ReesaJohn commented 5 years ago

Hey Joshua, I created a private github repo with an example of the issue I was having and invited you to it. This is the link: https://github.com/ReesaJohn/PotentialPeriodicityProblem

joshuaulrich commented 5 years ago

Investigation in the private repo revealed the root cause of the issue was missing values in the date vector. For example:

d <- as.Date("2019-08-09") + c(0, 1, NA, 3, 4)
xts::periodicity(d)
# Error in try.xts(x, error = "'x' needs to be timeBased or xtsible") : 
#   'x' needs to be timeBased or xtsible

So the proposed patch is not necessary, and could potentially be incorrect. At minimum, this error message should mention the actual error thrown. Then the user would know what needed to be fixed. For example:

xts::xts(NULL, d)
# Error in xts(x = NULL, order.by = x, ...) :
#     'order.by' cannot contain 'NA', 'NaN', or 'Inf'
joshuaulrich commented 1 year ago

I fixed this by throwing a warning about the NA values and then removing them.