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

Fix NA support in pivot_longer #831

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tdhock
Copy link

@tdhock tdhock commented Jan 20, 2025

Hi @markfairbanks
data.table implemented a change in its GitHub dev version recently, Rdatatable/data.table#6167
and our revdep checks told us that this change causes a new test failure in tidytable,

* checking tests ...
  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  The following object is masked from 'package:base':
  
      %in%
  
  > 
  > test_check("tidytable")
  [ FAIL 1 | WARN 0 | SKIP 0 | PASS 1335 ]
  
  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-pivot_longer.R:229:3'): can drop the 'id' column by specifying NA ──
  Names of `out` ('.id', 'x', 'y') don't match 'x', 'y'
  
  [ FAIL 1 | WARN 0 | SKIP 0 | PASS 1335 ]
  Error: Test failures

Above is only reproducible using the dev version of data.table from Github (not using current CRAN release).
This PR submits a fix, so that tidytable should work with the new data.table code.
Can you please review, merge, and then submit a new version of tidytable to CRAN?
We would like to submit a new version of data.table to CRAN in the next few weeks, and CRAN requires data.table to ensure compatibility with all of its reverse dependencies (such as tidytable).
Thanks!

@markfairbanks
Copy link
Owner

markfairbanks commented Jan 20, 2025

This looks like it's caused when trying to remove a column when the NULL value is in another variable and using the functional form of ':='(). Is this intended behavior?

library(data.table)

df <- data.table(x = 1:3, y = c("a", "a", "b"))

null_val <- NULL

# Works as expected
copy(df)[, x := null_val][]
#>         y
#>    <char>
#> 1:      a
#> 2:      a
#> 3:      b

# Functional forms have odd behavior
copy(df)[, ':='(x = null_val)][]
#>         x      y
#>    <list> <char>
#> 1: [NULL]      a
#> 2: [NULL]      a
#> 3: [NULL]      b
copy(df)[, let(x = null_val)][]
#>         x      y
#>    <list> <char>
#> 1: [NULL]      a
#> 2: [NULL]      a
#> 3: [NULL]      b

@tdhock
Copy link
Author

tdhock commented Jan 20, 2025

@MichaelChirico
Copy link

For now, we will avoid making the breaking change. But this will likely change in the future. Please follow Rdatatable/data.table#6740, Rdatatable/data.table#6167, and Rdatatable/data.table#5558 (I'm not sure now which of these threads will have the ongoing discussion, but anyway all three may be informative).

@markfairbanks
Copy link
Owner

@MichaelChirico I can make an update but this is confusing behavior (to me at least). These calls are effectively the same call that use let() but one of them gets the NULL value from a variable.

library(data.table)

df <- data.table(x = 1:3, y = c("a", "a", "b"))

null_val <- NULL

copy(df)[, let(x = NULL)][]
#>         y
#>    <char>
#> 1:      a
#> 2:      a
#> 3:      b
copy(df)[, let(x = null_val)][]
#>         x      y
#>    <list> <char>
#> 1: [NULL]      a
#> 2: [NULL]      a
#> 3: [NULL]      b

@MichaelChirico
Copy link

Thanks for the concise example, yea, that's pretty unappealing :)

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

Successfully merging this pull request may close these issues.

3 participants