Skip to content

fixes coerce from xts where a column x was present #4898

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

Merged
mattdowle merged 2 commits into from
May 25, 2021
Merged

fixes coerce from xts where a column x was present #4898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca2440f
fixes coerce from xts where a column x was preent, closes #4897
jangorecki Feb 11, 2021
9bb8ce4
added 'x' to news item and moved it up, test moved and simplified, en…
mattdowle May 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@

17. Assigning a wrong-length or non-list vector to a list column could segfault, [#4166](https://github.com/Rdatatable/data.table/issues/4166) [#4667](https://github.com/Rdatatable/data.table/issues/4667) [#4678](https://github.com/Rdatatable/data.table/issues/4678) [#4729](https://github.com/Rdatatable/data.table/issues/4729). Thanks to @fklirono, Kun Ren, @kevinvzandvoort and @peterlittlejohn for reporting, and to Václav Tlapák for the PR.

18. `as.data.table()` on `xts` objects containing a column named `x` would return an `index` of type plain `integer` rather than `POSIXct`, [#4897](https://github.com/Rdatatable/data.table/issues/4897). Thanks to Emil Sjørup for reporting, and Jan Gorecki for the PR.

## NOTES

1. New feature 29 in v1.12.4 (Oct 2019) introduced zero-copy coercion. Our thinking is that requiring you to get the type right in the case of `0` (type double) vs `0L` (type integer) is too inconvenient for you the user. So such coercions happen in `data.table` automatically without warning. Thanks to zero-copy coercion there is no speed penalty, even when calling `set()` many times in a loop, so there's no speed penalty to warn you about either. However, we believe that assigning a character value such as `"2"` into an integer column is more likely to be a user mistake that you would like to be warned about. The type difference (character vs integer) may be the only clue that you have selected the wrong column, or typed the wrong variable to be assigned to that column. For this reason we view character to numeric-like coercion differently and will warn about it. If it is correct, then the warning is intended to nudge you to wrap the RHS with `as.<type>()` so that it is clear to readers of your code that a coercion from character to that type is intended. For example :
Expand Down
2 changes: 1 addition & 1 deletion R/xts.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ as.data.table.xts = function(x, keep.rownames = TRUE, key=NULL, ...) {
if (identical(keep.rownames, FALSE)) return(r[])
index_nm = if (is.character(keep.rownames)) keep.rownames else "index"
if (index_nm %chin% names(x)) stop(domain=NA, gettextf("Input xts object should not have '%s' column because it would result in duplicate column names. Rename '%s' column in xts or use `keep.rownames` to change the index column name.", index_nm, index_nm))
r[, c(index_nm) := zoo::index(x)]
r[, c(index_nm) := zoo::index(x), env=list(x=x)]
Copy link
Copy Markdown
Member Author

@jangorecki jangorecki May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattdowle Great to use that you are using new feature. For future I would suggest to apply some styling preferences that will more easily convey which variables are substituted. So far I used to just prefix them with dot.

r[, c(index_nm) := zoo::index(.x), env=list(.x=x)]

but I am open for any other alternative. It is not really needed because there is no collision, but for readability it could be useful.

Copy link
Copy Markdown
Member

@mattdowle mattdowle May 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is a downside of this approach: the repetition of the variable names. Using .x actually means more like ..x already does and the . vs .. could be confusing; i.e. . conveys current level, and .. conveys one level up, but not here where env= is used if we follow this single . prefix style.

setcolorder(r, c(index_nm, setdiff(names(r), index_nm)))
# save to end to allow for key=index_nm
setkeyv(r, key)
Expand Down
5 changes: 5 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6714,6 +6714,10 @@ if (test_xts) {
" 6: 1970-01-07 6", " 7: 1970-01-08 7", " 8: 1970-01-09 8",
" 9: 1970-01-10 9", "10: 1970-01-11 10"))
options(old)

# as.data.table.xts(foo) had incorrect integer index with a column name called 'x', #4897
M = as.xts(matrix(1, dimnames=list("2021-05-23", "x")))
test(1465.19, inherits(as.data.table(M)$index,"POSIXct"))
Comment thread
mattdowle marked this conversation as resolved.

Sys.setenv("_R_CHECK_LENGTH_1_LOGIC2_" = TRUE)
}
Expand Down Expand Up @@ -17678,3 +17682,4 @@ test(2190.9, DT[1:2, a:=call('sum', 1)], error="type 'language' cannot be coerce
DT = data.table(i1 = c(234L, 250L, 169L, 234L, 147L, 96L, 96L, 369L, 147L, 96L), i4 = c(79L, 113L, 270L, -121L, 113L, 113L, -121L, 179L, -228L, 113L), v = 0)
test(2191, DT[1:5, sum(v), by=.(i5 = 1:5 %% 2L), verbose=TRUE], data.table(i5=1:0, V1=c(0,0)), output="gforce")