[R-package] GPL2 dependency reduction and some fixes #1401

Merged
merged 8 commits into from Jul 27, 2016

Projects

None yet

4 participants

@khotilov
Contributor
  • per #1338 : stringr ->stringi, ggplot backend separated
  • added base graphics plots to complement ggplot ones, added a few extras plot options, streamlined some tree path parsing code, general cleanup and tests
  • bug fix #1399 & test
  • bug fix for missing gblinear zero coefficients during dump parsing & test
  • fix R-package compilation in Linux
  • update the dmlc-core submodule reference to the current commit
@pommedeterresautee
Member

Needs to add stringi to DESCRIPTION and remove stringr.

* checking package dependencies ... ERROR
Namespace dependency not required:stringi’
`
@khotilov
Contributor

Linux travis uses gcc 4.6.3 and there are a couple of C++11 things in the new dmlc-core/include/dmlc/any.h that were introduced only in gcc 4.7 https://gcc.gnu.org/projects/cxx-status.html#cxx11:

The options could be either:

  • throw the R <= 3.2 for windows under the bus and have a minimum gcc 4.7 requirement
  • modify any.h to work with gcc 4.7 (use the standard member initialization, and add an ifdef conditional to use std::has_nothrow_copy_constructor with gcc 4.6 instead of std::is_copy_constructible )
    @tqchen what's your take?

I'll fix the R CMD check stuff after work.

@tqchen
Member
tqchen commented Jul 26, 2016

I will simply add a macro to disable any.h for g++ 4.6

@tqchen
Member
tqchen commented Jul 26, 2016

should be fixed by #1408 please see if rebase fixes the issue

@khotilov
Contributor

Great! That works too.

@tqchen
Member
tqchen commented Jul 27, 2016

@hetong007 can you also review this?

@hetong007 hetong007 commented on the diff Jul 27, 2016
R-package/R/xgb.plot.deepness.R
#' @export
-xgb.plot.deepness <- function(model = NULL) {
- if (!requireNamespace("ggplot2", quietly = TRUE)) {
- stop("ggplot2 package is required for plotting the graph deepness.",
- call. = FALSE)
- }
-
- if (!requireNamespace("igraph", quietly = TRUE)) {
- stop("igraph package is required for plotting the graph deepness.",
- call. = FALSE)
- }
+xgb.plot.deepness <- function(model = NULL, which = c("2x1", "max.depth", "med.depth", "med.weight"),
@hetong007
hetong007 Jul 27, 2016 Member

So far all the plotting functions have the same prefix xgb.plot. This change doesn't need to change the function for the sake of compatibility of the existing user scripts.

Can you rename xgb.plot.deepness as xgb.deepness, and please also rename xgb.ggplot.deepness as xgb.plot.deepness? If you want to keep the name of xgb.ggplot.deepness, you can add a function xgb.plot.deepness to simply call xgb.ggplot.deepness, so that even somehow we need to remove ggplot2 from xgboost, we don't need to change the exposed function name.

My opinion holds for xgb.plot.importance as well.

@khotilov
khotilov Jul 27, 2016 Contributor

Another option that I was initially considering was to keep only the same old xgb.plot.zzz function names but to have a backend = c("ggplot", "base") parameter to choose the backend, and there could be a global option to set a default one. I still personally prefer the current implementation - mainly because it's cleaner, simpler, and less coupled.

Semantically, function names xgb.plot.zzz and xgb.ggplot.zzz convey clear intent that they do plotting of zzz, and that they use the base and the ggplot facilities for that respectively. What you are proposing would be a rather arbitrary naming scheme which might cause more frustration in the long term, comparing to simply one time function name replacement by probably just a few users who would really need it (those who used the returned ggplot object from xgb.plot.importance in their scripts, I bet). The discovery of the xgb.ggplot.importance counterpart would be one click away in the help page after one sees an error. And I can make it more clear in the documentation that the previous function was renamed... Anyway, who would expect that pre-canned diagnostic graphing in a data mining package under active development would stay fixed? :)

But it's fairly easy to implement the single-function approach, and I'm open to suggestions. I just don't want to create more mess.

@hetong007
hetong007 Jul 27, 2016 Member

That makes sense to me. The single-function option is OK, but the current version separates ggplot2 better.

For most users, it is basically changing the old function's style. They will find what they need as long as the documentation is clear.

@hetong007 hetong007 merged commit d5c1433 into dmlc:master Jul 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hetong007
Member

Thanks! It is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment