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.

Sign up for GitHub

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

Jump to bottom

reclass() changes "tclass" attribute #249

Closed
Eluvias opened this issue Jul 16, 2018 · 9 comments · Fixed by #250
Closed

reclass() changes "tclass" attribute #249

Eluvias opened this issue Jul 16, 2018 · 9 comments · Fixed by #250
Labels
bug
Milestone
Release 0.11-1

Comments

@Eluvias
Copy link

Eluvias commented Jul 16, 2018 •
edited by joshuaulrich
Loading 

# 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
Copy link
Contributor

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

@TomAndrews
Copy link
Contributor

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

@joshuaulrich
Copy link
Owner

Thanks @TomAndrews, that's very helpful!

@joshuaulrich
Copy link
Owner

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

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

Eluvias commented Jul 17, 2018

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 referenced this issue Jul 17, 2018
@joshuaulrich
Avoid structure() call in .xts() constructor
50b2510 
Calls to structure() are relatively expensive, due to creation of
multiple temporary copies of the data. Replace the structure() call
with the logic from the ..xts() constructor.
@TomAndrews
Copy link
Contributor

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

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

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.

@TomAndrews TomAndrews mentioned this issue Jul 18, 2018
Merged
@joshuaulrich joshuaulrich mentioned this issue Jul 25, 2018
Closed
TomAndrews added a commit to TomAndrews/xts that referenced this issue Jul 27, 2018
@TomAndrews
Add unit tests for .xts() checking index class and timezone
beb4267 
As documented in joshuaulrich#249, v0.11 introduced changes to .xts() which altered
behaviour compared to v0.10-2.  As a first step to fixing this
regression, this commit adds unit tests for all possible permutations
for setting the index class and timezone in .xts().

The tests check accessing the class in 3 different ways:
    1. Using tclass()
    2. Using indexClass()
    3. Checking the 'tclass' attribute on the index object

Similarly the tests check accessing timezone by:
    1. Using tzone()
    2. Using indexTZ()
    3. Checking the '.indexTZ' attribute on the index object

These tests are designed to document the behaviour as of v0.10-2.  As
such they pass on v0.10-2 but fail for v0.11.
TomAndrews added a commit to TomAndrews/xts that referenced this issue Jul 27, 2018
@TomAndrews
Fix failure of test..xts_class
1d707c5 
Reinstate the behaviour as of v0.10-2.  .indexCLASS should take
precedence over tclass.

Fixes joshuaulrich#249.
@joshuaulrich joshuaulrich added this to the Release 0.11-1 milestone Jul 30, 2018
@TomAndrews TomAndrews mentioned this issue Jan 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
Release 0.11-1
Development

Successfully merging a pull request may close this issue.

Restore behaviour from 0.10-2 to .xts() TomAndrews/xts
3 participants
@joshuaulrich @TomAndrews @Eluvias