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

make 'merge.data.table' become an exported S3 method #2618

Closed
AndreMikulec opened this issue Feb 11, 2018 · 8 comments · Fixed by #3588
Closed

make 'merge.data.table' become an exported S3 method #2618

AndreMikulec opened this issue Feb 11, 2018 · 8 comments · Fixed by #3588
Assignees
Milestone

Comments

@AndreMikulec
Copy link

I can not use data.table:::merge.data.table in R CRAN packages.

data.table:::merge.data.table

is not exported and private access ":::" is not allowed in R CRAN packages.

Please make 'merge.data.table' become an exported S3 method.

Thanks.
Andre Mikulec

> library(data.table)
data.table 1.10.4.3
  The fastest way to learn (by data.table authors): https://www.datacamp.com/courses/data-analysis-the-data-table-way
  Documentation: ?data.table, example(data.table) and browseVignettes("data.table")
  Release notes, videos and slides: http://r-datatable.com

> methods(merge)
[1] merge.data.frame  merge.data.table* merge.default     merge.dendrogram*
see '?methods' for accessing help and source code
> data.table::merge.data.table
Error: 'merge.data.table' is not an exported object from 'namespace:data.table'
> data.table:::merge.data.table
function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FALSE, all.x = all,
                             all.y = all, sort = TRUE, suffixes = c(".x", ".y"), allow.cartesian=getOption("datatable.allow.cartesian"), ...)
> sessionInfo()
R version 3.4.3 (2017-11-30)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows 10 x64 (build 16299)

Matrix products: default

locale:
[1] LC_COLLATE=English_United States.1252
[2] LC_CTYPE=English_United States.1252
[3] LC_MONETARY=English_United States.1252
[4] LC_NUMERIC=C
[5] LC_TIME=English_United States.1252

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base

loaded via a namespace (and not attached):
 [1] httr_1.3.1        compiler_3.4.3    R6_2.2.2          tools_3.4.3
 [5] withr_2.1.1.9000  curl_3.1          memoise_1.1.0     data.table_1.10.5
 [9] knitr_1.19        git2r_0.21.0      digest_0.6.15     devtools_1.13.4
>
@AndreMikulec
Copy link
Author

Also, please export:

data.table:::duplicated.data.table.

Thanks.

@MarkusBonsch
Copy link
Contributor

Before exporting merge.data.table, we need to fix the following issues:
#2592
#2587
and update the documentation to reflect the differences in behaviour compared to merge.data.frame
#2593
I am currently working on that. Once this is fixed, I see no reason not to export.

@MarkusBonsch MarkusBonsch self-assigned this Feb 15, 2018
@arunsrinivasan
Copy link
Member

arunsrinivasan commented Feb 17, 2018

merge is an S3 generic in base R, and .data.table is a method for data.tables. Why are you using data.table:::merge.data.table? Use merge() by providing data.tables as arguments to the functions and they'd automatically dispatch the .data.table method.

@AndreMikulec
Copy link
Author

What I am saying is that I would like to S3 dispatch on merge.data.table without putting data.table on the search path. I would like to use double colons '::'. Triple colons ':::' are not allowed in R CRAN packages.

For example, I can do

> debug(zoo::as.Date)
> zoo::as.Date(17000)
debugging in: zoo::as.Date(17000)
debug at . . . /zoo/R/as.Date.R

But I can not do that in data.table merge

> debug(data.table:::merge.data.table)
> merge(data.table(BOD),data.table(BOD), by = "Time")
Error in data.table(BOD) : could not find function "data.table"

@jangorecki
Copy link
Member

Exporting method won't hurt us anyway and it is sometimes useful.

@mattdowle mattdowle added this to the 1.12.4 milestone May 17, 2019
@jangorecki
Copy link
Member

@AndreMikulec merge data.table is dispatched without attaching data.table to search path

d = data.table::data.table(a=1L, b=2L)
debug(data.table:::merge.data.table)
merge(d, d)

could you provide another use case where exporting merge method is necessary?

@AndreMikulec
Copy link
Author

AndreMikulec commented May 23, 2019

@jangorecki

case where exporting merge method is necessary?

From your example: suppose the 'd' is created in package 1 and by package 1 function f1. Suppose package 2 function f2 exists and receives as a parameter d. Suppose that in runnable code, function f2 receives as a parameter data.table d like

 f2 <- function(d) merge(d,d)
f2(d)

then package 2 (and function f2) relies on the dependence of package 1(that created d) and that 'package 2 depending on package 1'

But S3 is flexible. Using it to dispatch is a good idea (so I agree).

However, sometimes I want to be specific. I want to force a certain behavior. Or, I want to be sure that the function that I want is (at least first) called before a dispatch.

 f2 <- function(d) data.table::merge(d,d)

An example, is zoo::as.Date (that does S3 dispatch).
zoo::as.Date handles the "origin".
base::as.Date does not handle the origin.
Therefore, in my code, I prefer to use zoo::as.Date. I want zoo::as.Date to be called specifically (so that it is called first) and I want to put zoo::as.Date into a R CRAN package (that does not allow triple colons :::).

> as.Date(0)
Error in as.Date.numeric(0) : 'origin' must be supplied

> zoo::as.Date(0)
[1] "1970-01-01"

> as.Date(0)
Error in as.Date.numeric(0) : 'origin' must be supplied

Basically, if I call as.Date without zoo::, I am leaving myself open to errors. I do not know which as.Date method would be called. Also, if some package is on the search() path that has its own as.Date function, I am not sure if a 3rd as.Date method would be called. I would have be sure of loading order and dispatch order. What is the call? Is it base::as.Date or is it zoo::as.Date or is it package 3 as.Date? I do not want to spend my time chasing down (my own) loading order and dispatch order mistakes.

If this response seems harsh ( if it seems that way ) to anyone, then please forgive me?

My situation is very difficult to explain. My experience is from working with a very large code set with typically 50-100 imported packages.

Thanks.
I hope that helps.

@jangorecki
Copy link
Member

Yes, that helps, thank you.

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 a pull request may close this issue.

5 participants