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

tclass of xts changes in various functions #322

Closed TomAndrews closed 4 years ago

TomAndrews commented 4 years ago

Description

I've come across a few cases in xts 0.12 where tclass gets changed:

> test <- .xts(1:5, 1:5, tclass="timeDate")
> tclass(test > 2)
 [1] "POSIXct" "POSIXt"

and

 > test <- .xts(1:5, 1:5, tclass="timeDate")
 > tclass(reclass(1:5, test))
 [1] "POSIXct" "POSIXt"
 >

This is a regression of #249 in 0.12

I think there are a few places where .indexClass was passed into the .xts constructor before and tclass needs adding in its place.

Expected behavior

tclass should not be adjusted

Session Info

R version 3.6.2 (2019-12-12)
 Platform: x86_64-pc-linux-gnu (64-bit)
 Running under: Debian GNU/Linux 9 (stretch)

 Matrix products: default
 BLAS:   /usr/lib/libblas/libblas.so.3.7.0
 LAPACK: /usr/lib/lapack/liblapack.so.3.7.0

 locale:
  [1] LC_CTYPE=en_GB.UTF-8       LC_NUMERIC=C
  [3] LC_TIME=en_GB.UTF-8        LC_COLLATE=en_GB.UTF-8
  [5] LC_MONETARY=en_GB.UTF-8    LC_MESSAGES=en_GB.UTF-8
  [7] LC_PAPER=en_GB.UTF-8       LC_NAME=C
  [9] LC_ADDRESS=C               LC_TELEPHONE=C
 [11] LC_MEASUREMENT=en_GB.UTF-8 LC_IDENTIFICATION=C

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

 other attached packages:
 [1] xts_0.12-0 zoo_1.8-7

 loaded via a namespace (and not attached):
 [1] compiler_3.6.2  grid_3.6.2      lattice_0.20-38
joshuaulrich commented 4 years ago

Thanks for the report! I would be great if you could come up with a regression test for each instance you've identified where tclass needs to be passed to the .xts() constructor.

philaris commented 4 years ago

Thanks for the nice package! I created some tests, as you can see in my pull request.

joshuaulrich commented 4 years ago

This is a regression of #249 in 0.12

@TomAndrews, we were so focused on adding tests for the .xts() constructor that we didn't add any for the actual bug @Eluvias reported! We would have caught this if we had a regression test for reclass(). I'll make sure one is added.

@philaris thanks for the kind words and the contribution!

joshuaulrich commented 4 years ago

I fixed the specific cases mentioned in this issue. Please open a new issue if you find more cases that I missed. Thanks for the report @TomAndrews, and the tests @philaris!