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

tclass of xts changes in various functions #322

Closed
TomAndrews opened this issue Jan 21, 2020 · 4 comments · Fixed by #323
Closed

tclass of xts changes in various functions #322

TomAndrews opened this issue Jan 21, 2020 · 4 comments · Fixed by #323
Labels
Milestone

Comments

@TomAndrews
Copy link
Contributor

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

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 added a commit to philaris/xts that referenced this issue Jan 29, 2020
These tests guarantee that application of a relational operator on an xts
time series does not change its time class (tclass).

See joshuaulrich#322.
@philaris
Copy link
Contributor

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

@joshuaulrich
Copy link
Owner

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 added a commit that referenced this issue Apr 4, 2020
Ensure relational operators on an xts with a POSIXct index does not
change the 'tclass' attribute.

See #322.
joshuaulrich added a commit that referenced this issue Apr 4, 2020
The result of reclass() always returned an object with a POSIXct
tclass, even if 'match.to' did not have a POSIXct tclass. The test,
test.reclass_preserves_match.to_tclass(), fails if the changes to xts.R
are not applied.

See #322.
@joshuaulrich
Copy link
Owner

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants