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

do_rbind_xts(): Date vs POSIXct possible error #320

Closed ghost closed 4 years ago

ghost commented 4 years ago

This piece looks wrong:

https://github.com/joshuaulrich/xts/blob/master/src/rbind.c#L84

  PROTECT(xindex = getAttrib(x, xts_IndexSymbol)); P++;
  PROTECT(yindex = getAttrib(y, xts_IndexSymbol)); P++;

  if( TYPEOF(xindex) != TYPEOF(yindex) ) 
  {
    PROTECT(xindex = coerceVector(xindex, REALSXP)); P++;
    PROTECT(yindex = coerceVector(yindex, REALSXP)); P++;
  }

What if index(x) is a Date class and index(y) is a POSIXct class? And if the index is as Date class, but as a double, we will also get an incorrect result. Am I right?

Date - days from 1970-01-01 POSIXct- seconds from 1970-01-01

joshuaulrich commented 4 years ago

TYPEOF tells you whether the index is INTSXP or REALSXP. The class of the index is ignored. So both indices will be converted to REALSXP unless both are the same type.

The xts index is always seconds from 1970-01-01. But it is not always a double. For example:

str(.index(.xts(1, 1L)))
#  int 1
#  - attr(*, "tclass")= chr [1:2] "POSIXct" "POSIXt"
#  - attr(*, "tzone")= chr ""

When the index class is Date, the xts index is set to the number of seconds from the origin, at midnight UTC. For example, 1970-01-02 is 86400 seconds from the origin (also note that the index is numeric, even though it's a Date).

str(.index(xts(1, .Date(1))))
#  num 86400
#  - attr(*, "tzone")= chr "UTC"
#  - attr(*, "tclass")= chr "Date"

tl;dr: I think this code is okay.

ghost commented 4 years ago

Ok, I see that this change is taking place here. As long as I don't build xts objects outside of xts(), there should be no risk that rbind will return the garbage to me. So I close it.

Best