xgboost #43

Athospd · 2019-04-08T05:11:27Z

Overall

for object of class xgb.Booster

tidypredict_fit()
tidypredict_sql()
tidypredict_to_column()
parse_model()
build_fit_formula_xgb()

It is based on ´xgb,dump(dump_format = "text")` output. I tried to stick with the randomForest code structure as similar as I could.

todo

multiclass objectives,
vignettes,
tests with different datasets and different ntree sizes.

Comments

Custom objectives are returning linear because of a bug at xgb.train() that do not store objective when custom function is passed by user.
tidypredict_fit() returns a call instead of list (like randomForest). This way it is possible to make tidypredict_to_column() to work in despite it been a tree based model.

edgararuiz-zz · 2019-04-08T15:59:25Z

@Athospd - Thank you so much for this PR!! I'll review it soon.

Athospd · 2019-04-08T23:58:21Z

Two more things. I Just made a test with a real model from my work and It took hours. Maybelline It worth It to invest a little bit into efficiency.
And I forgot to inform the xgboost version. I'm affraid It Will impact a lot because the code depends on object names (and I already saw difference between xgboost from my personal Pc and my work's Pc)

Athospd · 2019-04-09T01:29:34Z

package ‘xgboost’ version 0.82.1 (latest from CRAN)

edgararuiz-zz · 2019-04-20T22:25:19Z

Hi @Athospd , I'm getting a test failure when I run your code locally. Do R checks fail on your side?

x |  1 1     | test-list [1.6 s]
----------------------------------------------------------------------------------
test-list.R:10: error: Supports parsed models in list objects
Only strings can be converted to symbols

I think the issue are changes made to the R/model-rf.R file.

edgararuiz-zz · 2019-04-20T22:28:20Z

R/model-xgboost.R

+    dplyr::mutate_at(dplyr::vars(Yes, No, Missing), ~stringr::str_replace(., "^.*-", "")) %>%
+    dplyr::mutate_at(dplyr::vars(Yes, No, Missing), ~as.integer(.) + 1) %>% # xxgboost is 0-indexed
+    dplyr::group_by(Tree) %>%
+    tidyr::nest(.key = "tree_data")


Would it be possible to avoid dependencies on tidyr please? I'm also trying to reduce dependencies on dplyr, but if we can't avoid those, we'll need to add the @importFrom for them in the R/tidypredict.R

edgararuiz-zz · 2019-04-20T22:28:43Z

R/model-xgboost.R

+}
+
+get_xgb_trees.character <- function(xgb_dump_text_with_stats, feature_names) {
+  feature_names_tbl <- tibble::enframe(feature_names, "Feature", "feature_name") %>% dplyr::mutate(Feature = as.character(Feature - 1))


Can we avoid dependencies on tibble please?

edgararuiz-zz · 2019-04-20T22:30:31Z

tests/testthat/test_xgboost.R

+library(magrittr)
+library(RSQLite)
+library(DBI)
+


I believe that these can be library calls can be moved to tests/testthat.R

R/model-rf.R

edgararuiz-zz · 2019-04-20T22:32:40Z

R/model-xgboost.R

+
+  trees <- xgb.model.dt.tree(text = xgb_dump_text_with_stats) %>%
+    dplyr::left_join(feature_names_tbl, by = "Feature") %>%
+    dplyr::mutate_at(dplyr::vars(Yes, No, Missing), ~stringr::str_replace(., "^.*-", "")) %>%


Can we remove the stringr dependency please?

I made a "base" version of get_xgb_trees.character() at this commit db1a933

edgararuiz-zz · 2019-04-20T22:38:51Z

For some reason, my Review comments were lost. Just wanted to say thank you for this PR. And that my comments were mainly centered around avoiding the use of tidyverse packages (excluding purrr) for transforming data within internal function. dplyr command should only be used to manipulate tbl_sql() tables, but not local data frames. I've had issues in the past with dependencies on packages, such as tibble, that made me remove all but the few that are in the R/tidypredict.R file. Thanks!!

Athospd · 2019-04-21T03:58:27Z

Thanks, Edgar, contributing for this pkg is a honor to me.
For some motivation: I currently work as data scientist on the biggest bank of Latin America and two models were put in production using your work in this project.

edgararuiz-zz · 2019-04-21T14:39:56Z

That's just awesome!! I'm glad that you are able to take advantage of this package.

At this point the Travis CI checks are still failing. But it looks that at this point is because it's missing the package dependency for xgboost: https://travis-ci.org/tidymodels/tidypredict/jobs/522636270#L2648. To solve that, you just need to add the dependency to the DESCRIPTION file, under "Suggests".

It looks like we'll almost there. So would you mind also adding the NEWS item please? Please make sure to reference your GH user and Issue ID that this PR covers at the end and in parenthesis, like this:

[Description of your change] (@Athospd, xgboost #43)

Athospd · 2019-04-22T02:51:40Z

I don't know if I'm missing something but I was only able to make the tests run successfully after put back the 'library' calls inside the test_xgboost.R file. I'd put it inside testthat.R as we'd agreed to do so. And maybe that explains why Travis CI checks were failing.

edgararuiz-zz · 2019-04-22T13:07:00Z

Are you getting any Warnings? I think the current failure is because Travis is set to treat Warnings as Errors
https://travis-ci.org/tidymodels/tidypredict/jobs/522887918#L4710

edgararuiz-zz · 2019-04-22T14:55:04Z

Ok, I just ran devtools::check() on your PR and these are the results:

-- R CMD check results ---------------------------------- tidypredict 0.3.0 ----
Duration: 46.6s

> checking for unstated dependencies in 'tests' ... WARNING
  'library' or 'require' call not declared from: 'magrittr'

> checking R code for possible problems ... NOTE
  get_xgb_trees.character: no visible global function definition for
    'xgb.model.dt.tree'
  get_xgb_trees.xgb.Booster: no visible global function definition for
    'xgb.dump'
  Undefined global functions or variables:
    xgb.dump xgb.model.dt.tree

0 errors v | 1 warning x | 1 note x
Error: R CMD check found WARNINGs
Execution halted

Exited with status 1.

Do we need magrittr to be called if dplyr is being called already?
For the NOTE, I think you just need to append those calls as a Global Variable here: https://github.com/tidymodels/tidypredict/blob/master/R/tidypredict.R#L16

edgararuiz-zz · 2019-04-23T13:37:52Z

tests/testthat.R

+library(xgboost)
+library(purrr)
+library(dplyr)
+library(magrittr)


I think we need to remove any references to magrittr completely. Unless you are using a function that dplyr does not import from that package. Just reviewed the Travis logs and it seems that the warning is being generated by not having this package listed as a dependency in the DESCRIPTION file:

* checking for unstated dependencies in â€˜testsâ€™ ... WARNING 'library' or 'require' call not declared from: â€˜magrittrâ€™ * checking tests ... Running â€˜testthat.Râ€™ [12s/12s] OK * checking PDF version of manual ... OK * DONE Status: 1 WARNING

sorry, this change was hung on my local branch.

… xgboost

edgararuiz-zz · 2019-04-24T03:03:21Z

Outstanding work @Athospd ! Thank you again.

github-actions · 2021-03-06T00:28:09Z

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

Athospd added 5 commits April 7, 2019 17:42

some pkg prefix

c65cf1d

xgboost tests

3842c61

xgboost

598b257

small fix in tests

fcd7e03

add test for 'tidypredict_to_column'

2af5559

edgararuiz-zz self-assigned this Apr 8, 2019

as.numeric

9171eed

edgararuiz-zz reviewed Apr 20, 2019

View reviewed changes

Athospd added 3 commits April 21, 2019 00:09

transfered library calls from test_xgboost to testthat

ee122ea

undo changes on get_rf_path

1cde286

get_xgb_trees.character version without dplyr and tidyr dependencies

db1a933

Athospd added 4 commits April 21, 2019 22:42

xgboost pkg in Suggests

6e0c84c

xgboost listed in NEWS

8d80089

bug fix. coercing data.table as data.frame

532745e

put library calls inside test_xgboost to make the tests run

b034219

add dbDisconnect() at test_xgboost.R

2fbf254

Athospd added 3 commits April 22, 2019 22:28

put xgboost:: before functions

0390dfb

-library(magrittr)

3760fee

-library(magrittr)

f596867

edgararuiz-zz reviewed Apr 23, 2019

View reviewed changes

Athospd added 3 commits April 23, 2019 23:08

Merge branch 'xgboost' of https://github.com/Athospd/tidypredict into…

45acd9e

… xgboost

-library(magrittr)

f192db4

Merge branch 'xgboost' of https://github.com/Athospd/tidypredict into…

8d9d6f6

… xgboost

edgararuiz-zz merged commit 50df616 into tidymodels:master Apr 24, 2019

edgararuiz-zz mentioned this pull request Apr 24, 2019

xgboost translation #40

Closed

Athospd deleted the xgboost branch April 24, 2019 03:10

github-actions bot locked and limited conversation to collaborators Mar 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xgboost #43

xgboost #43

Athospd commented Apr 8, 2019

edgararuiz-zz commented Apr 8, 2019

Athospd commented Apr 8, 2019

Athospd commented Apr 9, 2019 •

edited

edgararuiz-zz commented Apr 20, 2019

edgararuiz-zz Apr 20, 2019

edgararuiz-zz Apr 20, 2019

edgararuiz-zz Apr 20, 2019

edgararuiz-zz Apr 20, 2019

Athospd Apr 21, 2019 •

edited

edgararuiz-zz commented Apr 20, 2019

Athospd commented Apr 21, 2019 •

edited

edgararuiz-zz commented Apr 21, 2019

Athospd commented Apr 22, 2019 •

edited

edgararuiz-zz commented Apr 22, 2019

edgararuiz-zz commented Apr 22, 2019

edgararuiz-zz Apr 23, 2019

Athospd Apr 24, 2019

edgararuiz-zz commented Apr 24, 2019

github-actions bot commented Mar 6, 2021

xgboost #43

xgboost #43

Conversation

Athospd commented Apr 8, 2019

Overall

todo

Comments

edgararuiz-zz commented Apr 8, 2019

Athospd commented Apr 8, 2019

Athospd commented Apr 9, 2019 • edited

edgararuiz-zz commented Apr 20, 2019

edgararuiz-zz Apr 20, 2019

Choose a reason for hiding this comment

edgararuiz-zz Apr 20, 2019

Choose a reason for hiding this comment

edgararuiz-zz Apr 20, 2019

Choose a reason for hiding this comment

edgararuiz-zz Apr 20, 2019

Choose a reason for hiding this comment

Athospd Apr 21, 2019 • edited

Choose a reason for hiding this comment

edgararuiz-zz commented Apr 20, 2019

Athospd commented Apr 21, 2019 • edited

edgararuiz-zz commented Apr 21, 2019

Athospd commented Apr 22, 2019 • edited

edgararuiz-zz commented Apr 22, 2019

edgararuiz-zz commented Apr 22, 2019

edgararuiz-zz Apr 23, 2019

Choose a reason for hiding this comment

Athospd Apr 24, 2019

Choose a reason for hiding this comment

edgararuiz-zz commented Apr 24, 2019

github-actions bot commented Mar 6, 2021

Athospd commented Apr 9, 2019 •

edited

Athospd Apr 21, 2019 •

edited

Athospd commented Apr 21, 2019 •

edited

Athospd commented Apr 22, 2019 •

edited