[R-package] add a tree plotting function #6729

fboudry · 2024-11-23T22:20:59Z

Feature requested in #1222

Added a R function to plot trees.
Basically used the code posted in #1222 by @SpeckledJim2 and followed the given instruction.

Added DiagrammeR as suggested in DESCRIPTION Added lgb.plot.tree in _pkgdown.yml Roxygenized.

fboudry · 2024-11-23T22:29:22Z

@microsoft-github-policy-service agree

jameslamb

Thanks for your interest in LightGBM.

As I mentioned in the discussion on #1222, I'm supportive of trying to add something like this (especially since xgboost has it as well).

But I hope you'll see from the first round of suggestions I left here... significant work remains before I'd support merging this change into the package. If you are willing to work with us on this and go through multiple rounds of reviews and suggestions, we'd be grateful for the help! But if you don't have the time/interest to get this ready for inclusion in the package, please let me know and we'll close this PR and leave #1222 open for someone else to pick up.

jameslamb · 2024-11-24T19:06:46Z

R-package/DESCRIPTION

@@ -49,7 +49,8 @@ Suggests:
    markdown,
    processx,
    RhpcBLASctl,
-    testthat
+    testthat,
+    DiagrammeR


please keep this list in alphabetical order (move DiagrammeR to the top of this list).

You will also need to add DiagrammeR to every place in continuous integration scripts that installs optional dependencies for the project. You can find those like this:

git grep processx

jameslamb · 2024-11-24T19:07:43Z

R-package/R/lgb.plot.tree.R

@@ -0,0 +1,184 @@
+#' @name lgb.plot.tree
+#' @title Plot a single LightGBM tree using DiagrammeR.


Suggested change

#' @title Plot a single LightGBM tree using DiagrammeR.

#' @title Plot a single LightGBM tree.

Let's simplify this, please.

jameslamb · 2024-11-24T19:08:22Z

R-package/R/lgb.plot.tree.R

+
+# function to plot a single LightGBM tree using DiagrammeR


Suggested change

# function to plot a single LightGBM tree using DiagrammeR

We do not need to repeat in a comment here the same information that's already in the roxygen comments.

jameslamb · 2024-11-24T19:10:37Z

R-package/R/lgb.plot.tree.R

+    if (!inherits(model, "lgb.Booster")) {
+        stop("model: Has to be an object of class lgb.Booster")
+    }


Suggested change

if (!inherits(model, "lgb.Booster")) {

stop("model: Has to be an object of class lgb.Booster")

}

if (!.is_Booster(x = model)) {

stop("lgb.plot.tree: model should be an ", sQuote("lgb.Booster"))

}

Please follow the patterns used elsewhere in the library for this:

LightGBM/R-package/R/lgb.restore_handle.R

Lines 42 to 44 in 83c0ff3

if (!.is_Booster(x = model)) {

stop("lgb.restore_handle: model should be an ", sQuote("lgb.Booster"))

}

jameslamb · 2024-11-24T19:15:37Z

R-package/R/lgb.plot.tree.R

+        stop("tree: Has to be an integer numeric")
+    }
+    # extract data.table model structure
+    dt <- lgb.model.dt.tree(model)


Suggested change

dt <- lgb.model.dt.tree(model)

modelDT <- lgb.model.dt.tree(model)

Please don't use the name dt. That is a function in the {stats} package (for finding the density of a t-distribution)... try ?dt to see that.

Shadowing names from the standard library can lead to confusing errors. Please use modelDT as the name for this data.table instead.

jameslamb · 2024-11-24T20:06:26Z

R-package/R/lgb.plot.tree.R

+        nodes_df = nodes,
+        edges_df = edges,
+        attr_theme = NULL
+    ) %>%


In this project, by convention we:

do not use the %>% operator

use comma-first style everywhere

Please update this code and all the other code you're adding to follow that. Keeping all of the code looking the same across the codebase helps us to develop and review changes.

xgboost's implementation of similar functionality might be useful as a reference. See https://github.com/dmlc/xgboost/blob/e988b7cf1515b08ad0f949c26beb043ce0b33fe8/R-package/R/xgb.plot.tree.R#L159-L181

jameslamb · 2024-11-24T20:07:41Z

R-package/tests/testthat/test_lgb.plot.tree.R

@@ -0,0 +1,59 @@
+test_that("lgb.plot.tree works as expected"){


Please also add tests for the other types of machine learning tasks LightGBM can be used for:

binary classification

multiclass classification (where, please note, there are num_classes trees produced per iteration)

learning-to-rank

And for the following model situations:

uses categorical features

These are all cases that could affect the code as written... for example, categorical features have different splitting rules.

jameslamb · 2024-11-24T20:08:48Z

R-package/R/lgb.plot.tree.R

+    dt[, Value := 0.0]
+    dt[, Value := leaf_value]


Suggested change

dt[, Value := 0.0]

dt[, Value := leaf_value]

dt[, Value := leaf_value]

I don't understand this... what's the purpose of setting all rows to 0.0 and then immediately overwriting them? It seems to me that the 0.0 could probably be removed.

jameslamb · 2024-11-24T20:09:27Z

R-package/R/lgb.plot.tree.R

+    dt[is.na(Value), Value := internal_value]
+    dt[is.na(Gain), Gain := leaf_value]
+    dt[is.na(Feature), Feature := "Leaf"]
+    dt[, Cover := internal_count][Feature == "Leaf", Cover := leaf_count]
+    dt[, c("leaf_count", "internal_count", "leaf_value", "internal_value") := NULL]
+    dt[, Node := split_index]
+    max_node <- max(dt[["Node"]], na.rm = TRUE)
+    dt[is.na(Node), Node := max_node + leaf_index + 1]
+    dt[, ID := paste(Tree, Node, sep = "-")]
+    dt[, c("depth", "leaf_index") := NULL]
+    dt[, parent := node_parent][is.na(parent), parent := leaf_parent]
+    dt[, c("node_parent", "leaf_parent", "split_index") := NULL]
+    dt[, Yes := dt$ID[match(dt$Node, dt$parent)]]
+    dt <- dt[nrow(dt):1, ]
+    dt[, No := dt$ID[match(dt$Node, dt$parent)]]
+    # which way do the NA's go (this path will get a thicker arrow)
+    # for categorical features, NA gets put into the zero group
+    dt[default_left == TRUE, Missing := Yes]
+    dt[default_left == FALSE, Missing := No]


Can you please add some comments to make it a bit easier to understand what's happening in this wall of code? It's very difficult to read (at least for me) as currently written).

jameslamb · 2024-11-24T20:11:35Z

R-package/tests/testthat/test_lgb.plot.tree.R

+  # trees start from 0 in lgb.model.dt.tree
+  tree_table <- lgb.model.dt.tree(model)
+  expect_error({
+    lgb.plot.tree(model, 999)TRUE


Suggested change

lgb.plot.tree(model, 999)TRUE

lgb.plot.tree(model, 999)

This looks like it was included accidentally?

DiagrammeR in CI. Error messages. Default parameters. Changed tests.

…ree.R) Now tests regressions, binary, multiclass classification and ranks.

Commented code.

Added a warning to functions and shorter stop message to make tests work.

fboudry added 2 commits November 23, 2024 21:41

Added lgb.plot.tree function.

6862821

Added DiagrammeR as suggested in DESCRIPTION Added lgb.plot.tree in _pkgdown.yml Roxygenized.

Added tests.

0a7ea0e

fboudry requested review from guolinke, jameslamb, shiyu1994, jmoralez, borchero and StrikerRUS as code owners November 23, 2024 22:21

jameslamb changed the title ~~R tree plot~~ [R-package] add a tree plotting function Nov 24, 2024

jameslamb added in progress feature labels Nov 24, 2024

jameslamb requested changes Nov 24, 2024

View reviewed changes

jameslamb added the awaiting response label Dec 2, 2024

fboudry added 5 commits December 23, 2024 22:09

Merge branch 'microsoft:master' into R-tree-plot

5206b11

Added review suggestions.

757dc84

DiagrammeR in CI. Error messages. Default parameters. Changed tests.

Updated tests. (based on R-package/tests/testthat/test_lgb.model.dt.t…

55aba68

…ree.R) Now tests regressions, binary, multiclass classification and ranks.

Corrected error (missing comma) in the selected tree check (L66).

85ff97a

Commented code.

Corrected tests.

b4b648a

Added a warning to functions and shorter stop message to make tests work.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] add a tree plotting function #6729

[R-package] add a tree plotting function #6729

fboudry commented Nov 23, 2024 •

edited

Loading

fboudry commented Nov 23, 2024

jameslamb left a comment

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

jameslamb Nov 24, 2024

		@@ -0,0 +1,184 @@
		#' @name lgb.plot.tree
		#' @title Plot a single LightGBM tree using DiagrammeR.

	#' @title Plot a single LightGBM tree using DiagrammeR.
	#' @title Plot a single LightGBM tree.

	if (!.is_Booster(x = model)) {
	stop("lgb.restore_handle: model should be an ", sQuote("lgb.Booster"))
	}

	dt <- lgb.model.dt.tree(model)
	modelDT <- lgb.model.dt.tree(model)

		@@ -0,0 +1,59 @@
		test_that("lgb.plot.tree works as expected"){

	dt[, Value := 0.0]
	dt[, Value := leaf_value]
	dt[, Value := leaf_value]

[R-package] add a tree plotting function #6729

Are you sure you want to change the base?

[R-package] add a tree plotting function #6729

Conversation

fboudry commented Nov 23, 2024 • edited Loading

fboudry commented Nov 23, 2024

jameslamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboudry commented Nov 23, 2024 •

edited

Loading