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

[R] could XGBoosterPredict_R be removed? #8687

Closed
jameslamb opened this issue Jan 16, 2023 · 2 comments · Fixed by #8689
Closed

[R] could XGBoosterPredict_R be removed? #8687

jameslamb opened this issue Jan 16, 2023 · 2 comments · Fixed by #8689

Comments

@jameslamb
Copy link
Contributor

#6819 introduced a new R-to-C interface for predicting, replacing XGBoosterPredict_R().

As of that PR, XGBoosterPredict_R() is no longer used.

  • no R code in the R package calls its
  • none of the R package's tests call it

Would you support a PR to remove it?

Why I think it should be removed

I don't think that would be a user-facing breaking change, unless this project supports other R packages linking against the lib_xgboost.{so,dll,dylib} built for the R package, or supports invoking those R-to-C entrypoints directly from R code with :::.

And removing it would have the following benefits:

  • smaller built library size
  • faster compilation
  • reduced risk of spending time fixing warnings from static analyzers (esp. checks run by CRAN) about code that isn't actually used in the package

How I found this

used covr to find bits of code not covered by tests (click me)

Ran the following to generate a coverage report.

cd R-package/
Rscript -e "install.packages(c('covr', 'DT'), repos = 'https://cran.r-project.org')"
Rscript -e " \
    coverage <- covr::package_coverage(type = 'tests', quiet = FALSE);
    print(coverage);
    covr::report(coverage, file = file.path(getwd(), 'coverage.html'), browse = TRUE);
    "

That showed the following coverage:

xgboost Coverage: 84.73%
R/xgb.create.features.R: 0.00%
R/xgb.load.R: 40.00%
R/xgb.cv.R: 63.89%
R/xgb.save.R: 66.67%
R/xgb.DMatrix.save.R: 71.43%
R/xgb.DMatrix.R: 72.51%
R/xgb.plot.importance.R: 75.00%
R/xgb.plot.shap.R: 76.24%
R/xgb.ggplot.R: 77.05%
R/xgb.plot.deepness.R: 80.39%
R/utils.R: 82.24%
R/callbacks.R: 84.38%
R/xgb.dump.R: 85.71%
src/xgboost_custom.cc: 85.71%
R/xgb.Booster.R: 89.51%
R/xgb.load.raw.R: 90.00%
R/xgb.plot.tree.R: 91.67%
R/xgb.model.dt.tree.R: 92.11%
R/xgb.train.R: 92.21%
src/xgboost_R.cc: 92.29%
R/xgb.unserialize.R: 92.86%
R/xgb.importance.R: 93.33%
R/xgb.plot.multi.trees.R: 97.40%
R/xgb.config.R: 100.00%
R/xgb.save.raw.R: 100.00%
R/xgb.serialize.R: 100.00%
R/xgboost.R: 100.00%
src/init.c: 100.00%

And I saw in the clickable report that XGBoosterPredict_R is never called by any test code.

image
@trivialfis
Copy link
Member

Thank you for checking the code! Please remove the function.

At the time of writing the new function, I talked with others and was told that the C package is considered a part of the interface. I guess it's not really an issue for XGBoost since we never publish any formal document about the R-C packagae.

@jameslamb
Copy link
Contributor Author

Ok great, thanks very much! I'll put up a PR shortly.

since we never publish any formal document about the R-C package.

I agree with treating this as a private interface. I'd be surprised to learn that there are many use cases for others to link to the R-to-C library, instead of either using the C API directly or the R package directly.

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.

2 participants