reclass() changes "tclass" attribute

[Eluvias]

# v0.10-2
library(xts)

mat <- matrix(c(1, 1, 0.5, -1, 2, 1, 1, 1), ncol = 2)

xxts <- xts(mat,
            seq.Date(as.Date('2017-07-01'),
            by = 'year', length.out = 4))


obj <- reclass(mat, xxts)

all.equal(obj, xxts)
#> [1] TRUE

# v0.11-0
library(xts)

mat <- matrix(c(1, 1, 0.5, -1, 2, 1, 1, 1), ncol = 2)

xxts <- xts(mat,
            seq.Date(as.Date('2017-07-01'),
            by = 'year', length.out = 4))


obj <- reclass(mat, xxts)

all.equal(obj, xxts)
#> [1] "Attributes: < Component \"index\": Attributes: < Component \"tclass\": Lengths (2, 1) differ (string compare on first 1) > >"
#> [2] "Attributes: < Component \"index\": Attributes: < Component \"tclass\": 1 string mismatch > >"                                
#> [3] "Attributes: < Component \"tclass\": Lengths (2, 1) differ (string compare on first 1) >"                                     
#> [4] "Attributes: < Component \"tclass\": 1 string mismatch >"

[TomAndrews]

In case it helps, I have the same issue and bisecting shows it started with 50b2510

[TomAndrews]

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

[joshuaulrich]

Thanks @TomAndrews, that's very helpful!

[joshuaulrich]

@Eluvias can you provide more context for how you encountered this issue? If nothing else, it will help create unit tests to provide better coverage.

[joshuaulrich]

I've investigated and found this is due to how the duplicated index attributes (timezone, class, format) are handled in the .xts() constructor. That is why it affects reclass() and Ops.xts() (both specify .indexClass =).

Note that #245 will address this by removing the .index* attributes, and deprecating the relevant functions and function arguments.

An acceptable fix must handle the current behavior, until the .index* attributes and functions can be formally deprecated. "Current behavior" should be whatever the behavior was as-of v0.10-2. Tests should include all combinations of of specifications for tclass = and .indexClass =, as well as tzone = and .indexTZ = in the .xts() constructor.

I would greatly appreciate contributions of unit tests that pass under v0.10-2! Anyone (including future me) who wants to do some work on them should probably work from a branch created off the v0.10-2 tag.

[Eluvias]

It was captured through unit testing.
One of the functions affected was an implementation of ifelse() which handles multiple columns (#198).

x <- ifelse(coredata(data), yes, no)
out <- reclass(x, data)

[TomAndrews]

I just left a comment on 50b2510, apologies if I'm stating the obvious.

I had a quick look at setting up some unit tests but I'm afraid I'd need some slightly more idiot-proof instructions. Are you just after some tests of the output of .xts() with different permutations of tclass, .indexClass, tzone and .indexTZ? Or would it also need running through reclass/Ops.xts?

[joshuaulrich]

Sorry my suggestion wasn't clear. A user could have specified tclass in an .xts() call in their script or non-CRAN package. So what I had in mind was calling .xts() with the following combinations of tclass and .indexClass:

expand.grid(tclass = c("missing", "specified"),
            .indexClass = c("missing", "specified"))
#      tclass .indexClass
# 1   missing     missing
# 2 specified     missing
# 3   missing   specified
# 4 specified   specified

Where "specified" should be two different values (e.g. "Date" and "timeDate") when both tclass and .indexClass are specified, so you can tell which argument is being used in the result. And doing the same thing for tzone and .indexTZ, to be thorough.

No need to run through reclass() or Ops.xts(). The tests on .xts() should catch the behavior that is causing them to produce different output.

[TomAndrews]

I have pushed a branch on my fork which adds unit tests to the 0.10-2 tag. These all pass in 0.10-2 but there are some failures (as you'd expect) for 0.11.
https://github.com/TomAndrews/xts/tree/constructor-unit-tests

This doesn't merge cleanly with master but I have another branch with the tests and my attempts fixing the failures which I'll make a pull request for.