Skip to content

C data.table docs and minor rename#4753

Merged
mattdowle merged 5 commits intomasterfrom
cdt5
Apr 14, 2021
Merged

C data.table docs and minor rename#4753
mattdowle merged 5 commits intomasterfrom
cdt5

Conversation

@jangorecki
Copy link
Copy Markdown
Member

@jangorecki jangorecki commented Oct 12, 2020

Improved docs for recently provided header file for exported functions.

Note that there is a rename of exported C function.

Comment thread man/cdt.Rd
Comment on lines +15 to +19
dt = data.table::as.data.table(iris)
Rcpp::cppFunction("SEXP mysub2(SEXP x, SEXP rows, SEXP cols) { return DT_subsetDT(x,rows,cols); }",
include="#include <datatableAPI.h>",
depends="data.table")
mysub2(dt, 1:4, 1:4)
Copy link
Copy Markdown
Member Author

@jangorecki jangorecki Oct 12, 2020

Choose a reason for hiding this comment

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

This Dirk example is really nice and shows how new header can be nicely used. I would like to provide pure C example as well but cannot get it to work.

dt = data.table::as.data.table(iris)
mysub1 = inline::cfunction(sig=c(x="SEXP", rows="integer", cols="integer"),
  "return DT_subsetDT(x,rows,cols); ",
  includes = "#include <datatableAPI.h>",
  cppargs = paste0("-I",system.file("include", package="data.table")),
  language = "C")

@eddelbuettel any idea how to effectively use new header file for linking from C? if possible just base R, not even inline, for simplicity

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jangorecki Can you please rephrase that question? What is your are asking? Header files are using during compilation only (and we can all blame R's LinkingTo: for confusion). Are you aiming for an example in R using a header file but no compiler? I can't quite square it ...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Strictly speaking an example of importing DT's functions by importing API header rather than R_GetCCallable("data.table", "DT_subsetDT"). For use from C rather than C++.

Copy link
Copy Markdown
Contributor

@eddelbuettel eddelbuettel Oct 24, 2020

Choose a reason for hiding this comment

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

Are you asking if I have a package hanging around that uses the new C header? No, as it is still fairly new.

But you could take aim at for example RcppXts which was set up (well over seven and a half years ago !!) to do the same xts and the export header there.

It would be possible (and in fact very valuable) to have a similar examples package for data.table. But right now we have ... one function. Little stretched for an example package.

@jangorecki jangorecki linked an issue Oct 12, 2020 that may be closed by this pull request
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 12, 2020

Codecov Report

Merging #4753 (0a47514) into master (53e1585) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 0a47514 differs from pull request most recent head 84cdc4b. Consider uploading reports for the commit 84cdc4b to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4753   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files          73       73           
  Lines       14430    14430           
=======================================
  Hits        14347    14347           
  Misses         83       83           
Impacted Files Coverage Δ
src/init.c 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53e1585...84cdc4b. Read the comment docs.

Copy link
Copy Markdown
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Looks good to me (modulo the one inline comment). I'd have to run a test compilation to see "for sure" if all locations of the old identifier have been renamed but I trust @jangorecki on this.

@mattdowle mattdowle added this to the 1.14.1 milestone Apr 14, 2021
@mattdowle mattdowle merged commit 189be77 into master Apr 14, 2021
@mattdowle mattdowle deleted the cdt5 branch April 14, 2021 23:13
@jangorecki jangorecki modified the milestones: 1.14.9, 1.15.0 Oct 29, 2023
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.

organisation of C exports

3 participants