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

all.equal shouldn't ERROR when comparing a data.table to a non-data.table #4042

d-sci opened this issue Nov 12, 2019 · 0 comments · Fixed by #4072

all.equal shouldn't ERROR when comparing a data.table to a non-data.table #4042

d-sci opened this issue Nov 12, 2019 · 0 comments · Fixed by #4072


Copy link

@d-sci d-sci commented Nov 12, 2019

Currently gives a hard error when the second argument is not a data.table:

df <- data.frame(a=1)
dt <-
all.equal(df, dt)
#> [1] "Attributes: < Names: 1 string mismatch >"                                              
#> [2] "Attributes: < Length mismatch: comparison on first 2 components >"                     
#> [3] "Attributes: < Component \"class\": Lengths (1, 2) differ (string compare on first 1) >"
#> [4] "Attributes: < Component \"class\": 1 string mismatch >"                                
#> [5] "Attributes: < Component 2: Modes: numeric, externalptr >"                              
#> [6] "Attributes: < Component 2: target is numeric, current is externalptr >"
all.equal(dt, df)
#> Error in, df): 'target' and 'current' must both be data.tables

(the error is also thrown if the first argument is not a data.table, but that would only happen if you bypassed method dispatch, in which case it's your own fault)

This makes it impossible to use common paradigms such as if (isTRUE(all.equal(x, y)) {...} : I would have to throw a try in there just to handle the case that x (or one of its list elements or attributes) is a data.table and y (or its corresponding element/attribute) isn't. More generally, it seems bad that all.equal(x, y) could have such substantially different behaviour than all.equal(y, x).

This error is thrown near the very top of

if (! || ! 
        stop("'target' and 'current' must both be data.tables")

I see two possible and very simple remedies:

  1. Literally just replace stop with return (possibly tailoring the message to tell you which one isn't a data.table)
  2. Replace the whole stop clause with NextMethod() or explicit all.equal.default (such that all.equal(dt, df) would give approximately the same message as all.equal(df, dt).

Tests 1613.59 and 1613.60 explicitly check for this error, so maybe this is intentional, but consider this my request to change that. If I get a approval on either remedy, I could do the PR myself (it would be my first for an open source project, but seems simple enough!)

#> R version 3.5.2 (2018-12-20)
#> Platform: x86_64-pc-linux-gnu (64-bit)
#> Running under: Ubuntu 16.04.5 LTS
#> Matrix products: default
#> BLAS: /usr/lib/libblas/
#> LAPACK: /usr/lib/lapack/
#> locale:
#>  [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
#>  [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
#>  [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
#>  [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
#>  [9] LC_ADDRESS=C               LC_TELEPHONE=C            
#> attached base packages:
#> [1] stats     graphics  grDevices utils     datasets  methods   base     
#> other attached packages:
#> [1] data.table_1.12.2
#> loaded via a namespace (and not attached):
#>  [1] compiler_3.5.2  magrittr_1.5    tools_3.5.2     htmltools_0.4.0
#>  [5] yaml_2.2.0      Rcpp_1.0.3      stringi_1.4.3   rmarkdown_1.16 
#>  [9] highr_0.8       knitr_1.25      stringr_1.4.0   xfun_0.10      
#> [13] digest_0.6.22   rlang_0.4.1     evaluate_0.14
d-sci pushed a commit to d-sci/data.table that referenced this issue Nov 21, 2019
@mattdowle mattdowle added this to the 1.12.9 milestone Jan 8, 2020
@mattdowle mattdowle added the bug label Jan 8, 2020
@jangorecki jangorecki removed this from the 1.12.11 milestone May 26, 2020
@jangorecki jangorecki added this to the 1.12.9 milestone May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging a pull request may close this issue.

3 participants