Add capability to get possible max and min values for a model #2737

JoanFM · 2020-02-03T11:54:49Z

Proposal to provide feature described in: #2736

src/io/tree.cpp

JoanFM · 2020-02-03T14:43:15Z

Apparently It fails to compile at linking time, but I do not experience this error locally

guolinke · 2020-02-03T17:16:21Z

You could move the implementation from gbdt_prediction.cpp to gbdt.cpp .

… to upper and lower bound, move implementation to gdbt.cpp

JoanFM · 2020-02-04T07:45:06Z

Changed according to comments

StrikerRUS

@JoanFM Thanks for your PR! Please check some comments below:

include/LightGBM/c_api.h

include/LightGBM/tree.h

src/boosting/gbdt.cpp

include/LightGBM/c_api.h

Co-Authored-By: Nikita Titov <[email protected]>

…ython and a basic test

StrikerRUS · 2020-02-05T12:16:54Z

@JoanFM New asserts in basic test cause failures for all OSes:

>       self.assertAlmostEqual(bst.upper_bound(), 3.2911086148583832)
E       AssertionError: 3.3182142872462883 != 3.2911086148583832 within 7 places

I guess it's due to you have not the latest commit installed locally. Also, I think 4-6 digits after a dot will be enough to allow future contributions with insignificant changes will not require value adjustments.

…ax_min_leafs

JoanFM · 2020-02-05T12:58:32Z

Thank you for your comments @StrikerRUS , I have done the changes, I hope it helps

tests/python_package_test/test_basic.py

…ck to test with hardcoded value

…to max_min_leafs

…ax_min_leafs

StrikerRUS

Some minor comments before merging.

Also, ping @jameslamb as I'm a little bit confused by function names in R (trailing _).

src/c_api.cpp

src/io/tree.cpp

tests/python_package_test/test_basic.py

Co-Authored-By: Nikita Titov <[email protected]>

…to max_min_leafs

StrikerRUS

@JoanFM LGTM! Thanks a lot for your contribution!

guolinke · 2020-02-19T01:17:44Z

src/c_api.cpp

@@ -357,6 +357,16 @@ class Booster {
    return boosting_->FeatureImportance(num_iteration, importance_type);
  }

+  double UpperBoundValue() const {
+    std::lock_guard<std::mutex> lock(mutex_);


Why mutex is needed here?

to guarantee that this value is read while no one is changing any inner value

okay, that is why we need reader/writer locks.

jameslamb · 2020-02-20T16:50:17Z

R-package/R/lgb.Booster.R

+      lower_bound <- 0L
+      lgb.call(
+        "LGBM_BoosterGetLowerBoundValue_R"
+        , ret = upper_bound


I was too slow getting to review here, apologies! @guolinke I'm going to make a followup PR with some changes:

using trailing _ is very uncommon in R, these methods should be called upper_bound() and lower_bound()

there is a bug on this line...return of the call to LGBM_BoosterGetLowerBoundValue_R should go into lower_bound, not upper_bound

there should be unit tests added whenever we introduce new methods in the public API of the package

JoanFM requested review from chivee and guolinke as code owners February 3, 2020 11:54

JoanFM mentioned this pull request Feb 3, 2020

Upper and lower bound of the value a tree can give #2736

Closed

Add capability to get possible max and min values for a model

971720f

JoanFM force-pushed the max_min_leafs branch from c1b68eb to 971720f Compare February 3, 2020 13:51

guolinke reviewed Feb 3, 2020

View reviewed changes

src/io/tree.cpp Outdated Show resolved Hide resolved

Change implementation to have return value in tree.cpp, change naming…

3c49f4c

… to upper and lower bound, move implementation to gdbt.cpp

StrikerRUS requested changes Feb 4, 2020

View reviewed changes

JoanFM and others added 2 commits February 5, 2020 07:56

Update include/LightGBM/c_api.h

48a9474

Co-Authored-By: Nikita Titov <[email protected]>

Change iteration to avoid potential overflow, add bindings to R and P…

0d516c1

…ython and a basic test

JoanFM requested review from henry0312, jameslamb, Laurae2 and wxchan as code owners February 5, 2020 07:41

joanfontanals added 2 commits February 5, 2020 13:49

Merge branch 'master' of https://github.com/microsoft/LightGBM into m…

f14ca1f

…ax_min_leafs

Adjust test values

8c58493

JoanFM and others added 2 commits February 5, 2020 23:11

Consider const correctness and multithreading protection

019bda8

Update test values

c43bf1c

JoanFM force-pushed the max_min_leafs branch from b87221c to c43bf1c Compare February 6, 2020 13:50

JoanFM requested a review from StrikerRUS February 6, 2020 17:45

StrikerRUS reviewed Feb 6, 2020

View reviewed changes

tests/python_package_test/test_basic.py Outdated Show resolved Hide resolved

JoanFM force-pushed the max_min_leafs branch from c43bf1c to c6964dd Compare February 6, 2020 20:45

Update test values

cfa612b

JoanFM force-pushed the max_min_leafs branch from c6964dd to cfa612b Compare February 6, 2020 20:46

Add test to check that model is exactly the same in all platforms

d3a54ae

JoanFM force-pushed the max_min_leafs branch from 8153588 to 14a4603 Compare February 9, 2020 19:13

Do not check for num_leaves to be smaller than actual size and get ba…

fd0bdb8

…ck to test with hardcoded value

JoanFM force-pushed the max_min_leafs branch from 14a4603 to fd0bdb8 Compare February 9, 2020 20:07

JoanFM added 2 commits February 12, 2020 09:16

Merge branch 'max_min_leafs' of https://github.com/JoanFM/LightGBM in…

d6ca4ff

…to max_min_leafs

Merge branch 'master' of https://github.com/microsoft/LightGBM into m…

a753edf

…ax_min_leafs

JoanFM force-pushed the max_min_leafs branch from 640aa78 to a753edf Compare February 12, 2020 08:17

JoanFM added 2 commits February 12, 2020 09:46

Change test order

19234e0

Add gpu_use_dp option in test

00bdde1

JoanFM force-pushed the max_min_leafs branch from 9261d31 to 00bdde1 Compare February 13, 2020 08:26

Remove helper test method

82c677c

JoanFM requested a review from StrikerRUS February 13, 2020 17:46

JoanFM mentioned this pull request Feb 14, 2020

feat: Change locking strategy of Booster, allow for share and unique locks #2760

Merged

StrikerRUS requested changes Feb 15, 2020

View reviewed changes

JoanFM and others added 5 commits February 17, 2020 10:42

Update src/c_api.cpp

cb94fe1

Co-Authored-By: Nikita Titov <[email protected]>

Update src/io/tree.cpp

136237e

Co-Authored-By: Nikita Titov <[email protected]>

Update src/io/tree.cpp

c39c32f

Co-Authored-By: Nikita Titov <[email protected]>

Update tests/python_package_test/test_basic.py

a36be1e

Co-Authored-By: Nikita Titov <[email protected]>

Remoove imports

d8abaa7

JoanFM requested a review from StrikerRUS February 17, 2020 09:44

Merge branch 'max_min_leafs' of https://github.com/JoanFM/LightGBM in…

aa362e6

…to max_min_leafs

JoanFM force-pushed the max_min_leafs branch from 61c6dbe to aa362e6 Compare February 18, 2020 12:55

StrikerRUS approved these changes Feb 19, 2020

View reviewed changes

StrikerRUS requested a review from guolinke February 19, 2020 03:33

guolinke reviewed Feb 19, 2020

View reviewed changes

guolinke approved these changes Feb 20, 2020

View reviewed changes

guolinke merged commit 18e7de4 into microsoft:master Feb 20, 2020

jameslamb reviewed Feb 20, 2020

View reviewed changes

jameslamb mentioned this pull request Feb 20, 2020

[R-package] Fixed R implementation of upper_bound() and lower_bound() for lgb.Booster #2785

Merged

guolinke added the feature label Mar 1, 2020

lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add capability to get possible max and min values for a model #2737

Add capability to get possible max and min values for a model #2737

JoanFM commented Feb 3, 2020

JoanFM commented Feb 3, 2020

guolinke commented Feb 3, 2020

JoanFM commented Feb 4, 2020

StrikerRUS left a comment

StrikerRUS commented Feb 5, 2020

JoanFM commented Feb 5, 2020

StrikerRUS left a comment

StrikerRUS left a comment

guolinke Feb 19, 2020

JoanFM Feb 19, 2020

guolinke Feb 20, 2020

jameslamb Feb 20, 2020

Add capability to get possible max and min values for a model #2737

Add capability to get possible max and min values for a model #2737

Conversation

JoanFM commented Feb 3, 2020

JoanFM commented Feb 3, 2020

guolinke commented Feb 3, 2020

JoanFM commented Feb 4, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

StrikerRUS commented Feb 5, 2020

JoanFM commented Feb 5, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

StrikerRUS left a comment

Choose a reason for hiding this comment

guolinke Feb 19, 2020

Choose a reason for hiding this comment

JoanFM Feb 19, 2020

Choose a reason for hiding this comment

guolinke Feb 20, 2020

Choose a reason for hiding this comment

jameslamb Feb 20, 2020

Choose a reason for hiding this comment