[R-package] enable use of trees with linear models at leaves (fixes #3319) #3699

jameslamb · 2020-12-31T06:10:34Z

This PR closes #3319, adding support for the new "leaves with linear models" feature @btrotta added in #3299.

Thankfully, the concerns from #3319 (comment) about the order in constructing the dataset weren't an issue! lgb.train() in the R package calls update_params() on the Dataset object and constructs it (

LightGBM/R-package/R/lgb.train.R

Lines 146 to 147 in 967b45c

    
           data$update_params(params = params) 
        
           data$construct()

), so it will respect linear_trees = true. If you've already called $construct() on the dataset before passing it in to lgb.train(), you'll get an informative error telling you that you can't use an already-constructed Dataset with linear_trees = false in training with linear_trees = true.

The unit tests I added for lgb.train() were based on those added in https://github.com/microsoft/LightGBM/pull/3299/files#diff-263bfdd4e44ba860ef61b458adfab0d5f391766a27a3fa7e9f221afc5f1e5bad, except refit() because the R package does not currently support refit() (#2369).

…icrosoft#3319)

jameslamb · 2020-12-31T06:13:41Z

R-package/configure

@@ -1699,6 +1699,12 @@ CXX=`"${R_HOME}/bin/R" CMD config CXX11`
 # LightGBM-specific flags
 LGB_CPPFLAGS=""

+#########


/gha run r-valgrind

this build (https://github.com/microsoft/LightGBM/runs/1629232486?check_suite_focus=true) failed with an error I didn't see testing the CRAN package locally

In file included from ./include/Eigen/Dense:1,
from treelearner/linear_tree_learner.cpp:7:
./include/Eigen/Core:15:10: fatal error: src/Core/util/DisableStupidWarnings.h: No such file or directory
15 | #include "src/Core/util/DisableStupidWarnings.h"

Will look into it tomorrow. Probably just a mistake I made with the include paths.

/gha run r-valgrind

https://github.com/microsoft/LightGBM/runs/1632880373?check_suite_focus=true

/gha run r-valgrind

Maybe the problem is that the fatal error causes lightgbm to exit without freeing some allocated memory? Similar to this: https://stackoverflow.com/questions/1901322/valgrind-reports-memory-leak-when-assigning-a-value-to-a-string

good idea! I know we ran into similar issues in the past. I'm going to try pushing a commit right now that skips those tests, to test that theory.

/gha run r-valgrind

https://github.com/microsoft/LightGBM/actions/runs/481344404

nice work @btrotta ! After skipping the one test introduced in this PR that checks for errors, valgrind is happy 😀

==2177== ==2177== HEAP SUMMARY: ==2177== in use at exit: 312,459,948 bytes in 55,720 blocks ==2177== total heap usage: 2,837,681 allocs, 2,781,961 frees, 6,180,339,569 bytes allocated ==2177== ==2177== 336 bytes in 1 blocks are possibly lost in loss record 152 of 2,721 ==2177== at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==2177== by 0x40149CA: allocate_dtv (dl-tls.c:286) ==2177== by 0x40149CA: _dl_allocate_tls (dl-tls.c:532) ==2177== by 0x5706322: allocate_stack (allocatestack.c:622) ==2177== by 0x5706322: pthread_create@@GLIBC_2.2.5 (pthread_create.c:660) ==2177== by 0x56D4DDA: ??? (in /usr/lib/x86_64-linux-gnu/libgomp.so.1.0.0) ==2177== by 0x56CC8E0: GOMP_parallel (in /usr/lib/x86_64-linux-gnu/libgomp.so.1.0.0) ==2177== by 0x15576383: LGBM_DatasetCreateFromCSC (c_api.cpp:1305) ==2177== by 0x155A5512: LGBM_DatasetCreateFromCSC_R (lightgbm_R.cpp:91) ==2177== by 0x4942146: R_doDotCall (dotcode.c:634) ==2177== by 0x494CFDD: do_dotcall (dotcode.c:1281) ==2177== by 0x49A06DF: bcEval (eval.c:7072) ==2177== by 0x498C2C0: Rf_eval (eval.c:727) ==2177== by 0x498F02F: R_execClosure (eval.c:1897) ==2177== ==2177== LEAK SUMMARY: ==2177== definitely lost: 0 bytes in 0 blocks ==2177== indirectly lost: 0 bytes in 0 blocks ==2177== possibly lost: 336 bytes in 1 blocks ==2177== still reachable: 312,459,612 bytes in 55,719 blocks ==2177== of which reachable via heuristic: ==2177== newarray : 4,264 bytes in 1 blocks ==2177== suppressed: 0 bytes in 0 blocks ==2177== Reachable blocks (those to which a pointer was found) are not shown. ==2177== To see them, rerun with: --leak-check=full --show-leak-kinds=all ==2177== ==2177== For lists of detected and suppressed errors, rerun with: -s ==2177== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 0 from 0) writing valgrind output to valgrind-logs.log valgrind found 0 bytes definitely lost valgrind found 0 bytes indirectly lost valgrind found 336 bytes possibly lost

I think we should leave this skip in to get this feature merged, and I can work on removing it separately as a follow-up task.

jameslamb · 2020-12-31T17:51:51Z

I'm going to mark this work in progress, sorry. Some R CMD check things I need to fix. Will add reviewers back when it's ready.

StrikerRUS · 2021-01-02T23:01:38Z

include/LightGBM/config.h

@@ -155,7 +155,6 @@ struct Config {
  // descl2 = missing values must be encoded as ``np.nan`` (Python) or ``NA`` (CLI), not ``0``


@jameslamb Please add info about missing values representation in R-package.

sorry for the delay! Added in 302a71c

jameslamb · 2021-01-15T21:15:38Z

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/488906902

Status: success ✔️.

jameslamb · 2021-01-15T22:48:11Z

I've updated this branch with the latest master, and the branch is in a state where it shouldn't cause valgrind issues on CRAN (#3699 (comment)). I think this is ready for another review.

I'll also test on Solaris soon and report the results.

StrikerRUS

LGTM! Thanks for updating the docs! But I'm not R-reviewer, so I'd better leave my approval in a form of comment.

Just one question: maybe add a test for save-load linear booster in R?

jameslamb · 2021-01-16T19:10:12Z

LGTM! Thanks for updating the docs! But I'm not R-reviewer, so I'd better leave my approval in a form of comment.

Makes sense. @guolinke can you take a look when you have time?

Just one question: maybe add a test for save-load linear booster in R?

That's a good suggestion, thanks. I'll do that right now.

jameslamb · 2021-01-16T20:58:48Z

I just added two tests that check that the model can be saved and reloaded (one tests a text file, one .rds): 61d7b11

I also tested on Solaris with R Hub, and did not see any issues 😁

guolinke

LGTM

StrikerRUS · 2021-01-19T16:24:21Z

@jameslamb Can this branch be removed?

jameslamb · 2021-01-19T16:32:54Z

oh I thought I already had deleted it. Because i see "jameslamb deleted ..." on this PR

maybe I accidentally created a LightGBM branch with that same name. Yep, I'll delete it.

StrikerRUS · 2021-01-19T17:22:20Z

Yeah, this PR was from your fork according to jameslamb:feat/....

github-actions · 2023-08-24T02:18:35Z

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

jameslamb added 2 commits December 30, 2020 23:43

[R-package] enable use of trees with linear models at leaves (fixes m…

89474ed

…icrosoft#3319)

remove problematic pragmas

d9df9f5

jameslamb added the feature label Dec 31, 2020

jameslamb requested review from Laurae2, guolinke and btrotta December 31, 2020 06:10

jameslamb requested review from chivee, huanzhang12 and StrikerRUS as code owners December 31, 2020 06:10

jameslamb commented Dec 31, 2020

View reviewed changes

fix tests

64377d8

jameslamb removed request for chivee, huanzhang12, Laurae2, guolinke, StrikerRUS and btrotta December 31, 2020 17:51

jameslamb added the in progress label Dec 31, 2020

jameslamb changed the title ~~[R-package] enable use of trees with linear models at leaves (fixes #3319)~~ WIP: [R-package] enable use of trees with linear models at leaves (fixes #3319) Dec 31, 2020

jameslamb marked this pull request as draft January 1, 2021 05:58

jameslamb added 4 commits January 1, 2021 01:07

try to fix build scripts

0d4131d

try fixing pragma check

186bb08

more pragma checks

ea692a3

ok fix pragma stuff for real

924f5d9

jameslamb marked this pull request as ready for review January 1, 2021 22:47

jameslamb requested review from btrotta, guolinke and Laurae2 as code owners January 1, 2021 22:47

jameslamb mentioned this pull request Jan 1, 2021

[R-package] memory leaks detected by valgrind #3704

Closed

jameslamb changed the title ~~WIP: [R-package] enable use of trees with linear models at leaves (fixes #3319)~~ [R-package] enable use of trees with linear models at leaves (fixes #3319) Jan 1, 2021

jameslamb added awaiting review and removed in progress labels Jan 1, 2021

jameslamb added 2 commits January 1, 2021 17:05

empty commit

13aac4e

regenerate documentation

ea51005

StrikerRUS reviewed Jan 2, 2021

View reviewed changes

btrotta mentioned this pull request Jan 3, 2021

Initialize any_nan_ property of LinearTreeLearner #3709

Merged

jameslamb added 5 commits January 11, 2021 08:52

Merge branch 'master' into feat/r-linear-learner

58ec47d

try skipping test

8eb7bec

Merge branch 'master' into feat/r-linear-learner

1594255

uncomment CI

161cd6b

Merge branch 'master' into feat/r-linear-learner

3c3615d

add note on missing value types for R

302a71c

StrikerRUS reviewed Jan 16, 2021

View reviewed changes

add tests on saving and re-loading booster

61d7b11

Merge branch 'master' into feat/r-linear-learner

88d6c05

guolinke approved these changes Jan 18, 2021

View reviewed changes

StrikerRUS removed the awaiting review label Jan 18, 2021

StrikerRUS merged commit ed651e8 into microsoft:master Jan 18, 2021

StrikerRUS mentioned this pull request Jan 18, 2021

[R-package][tests] remove unused lines in tests for the speedup #3783

Merged

jameslamb deleted the feat/r-linear-learner branch January 18, 2021 15:46

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R-package] enable use of trees with linear models at leaves (fixes #3319) #3699

[R-package] enable use of trees with linear models at leaves (fixes #3319) #3699

jameslamb commented Dec 31, 2020

jameslamb Dec 31, 2020

jameslamb Dec 31, 2020

jameslamb Jan 1, 2021

jameslamb Jan 1, 2021

jameslamb Jan 1, 2021

btrotta Jan 12, 2021

jameslamb Jan 12, 2021

jameslamb Jan 12, 2021

jameslamb Jan 12, 2021

jameslamb Jan 13, 2021

jameslamb commented Dec 31, 2020

StrikerRUS Jan 2, 2021

jameslamb Jan 15, 2021

jameslamb commented Jan 15, 2021 •

edited by guolinke

Loading

jameslamb commented Jan 15, 2021

StrikerRUS left a comment

jameslamb commented Jan 16, 2021

jameslamb commented Jan 16, 2021

guolinke left a comment

StrikerRUS commented Jan 19, 2021

jameslamb commented Jan 19, 2021

StrikerRUS commented Jan 19, 2021

github-actions bot commented Aug 24, 2023

		@@ -155,7 +155,6 @@ struct Config {
		// descl2 = missing values must be encoded as ``np.nan`` (Python) or ``NA`` (CLI), not ``0``

[R-package] enable use of trees with linear models at leaves (fixes #3319) #3699

[R-package] enable use of trees with linear models at leaves (fixes #3319) #3699

Conversation

jameslamb commented Dec 31, 2020

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

jameslamb commented Dec 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb commented Jan 15, 2021 • edited by guolinke Loading

jameslamb commented Jan 15, 2021

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Jan 16, 2021

jameslamb commented Jan 16, 2021

guolinke left a comment

Choose a reason for hiding this comment

StrikerRUS commented Jan 19, 2021

jameslamb commented Jan 19, 2021

StrikerRUS commented Jan 19, 2021

github-actions bot commented Aug 24, 2023

jameslamb commented Jan 15, 2021 •

edited by guolinke

Loading