added support for case_weights by p-schaefer · Pull Request #72 · tidymodels/bonsai

p-schaefer · 2023-12-14T03:26:18Z

I noticed there wasn't much progress on integrating case_weights into lightgbm through bonsai so I've taken a crack at adding it. This is in reference to 65.

simonpcouch

A strong start! Some pointers to move this closer:

simonpcouch · 2023-12-15T14:57:43Z

 #' @keywords internal
 #' @export
-train_lightgbm <- function(x, y, max_depth = -1, num_iterations = 100, learning_rate = 0.1,
+train_lightgbm <- function(x, y,weights=NULL, max_depth = -1, num_iterations = 100, learning_rate = 0.1,


Suggested change

train_lightgbm <- function(x, y,weights=NULL, max_depth = -1, num_iterations = 100, learning_rate = 0.1,

train_lightgbm <- function(x, y, weights = NULL, max_depth = -1, num_iterations = 100, learning_rate = 0.1,

Spaces after commas and before and after operators; could you apply this style elsewhere in the PR?

https://style.tidyverse.org/syntax.html

I think I've corrected these

simonpcouch · 2023-12-15T14:58:17Z

  if (!is.null(val_index)) {
    args$main$valids <-
      list(validation =
-          lightgbm::lgb.Dataset(


There are a number of unrelated style edits in this PR like this one. Could you revert?

I think I've corrected these? Let me know if thats not the case.

Almost there. If you look in this diff, there are several spaces before the function call and arguments that didn't used to be there.

…er vector specifying one or more variables in `x`used for grouping observations with the same value to either the training or validation set.

…ate branch

p-schaefer · 2023-12-15T20:17:43Z

Thanks for the review @simonpcouch. Note, I am also working on a method that allows some ability to customize validation data beyond just specifying a proportion of training data. I find this quite necessary for my work (since my data are not entirely independent - hence needing case weights as well), but I don't know if its robust enough to include in Bonsai proper.

simonpcouch · 2023-12-22T16:39:34Z

 # this function ensures that multiclass classification predictions are always
 # returned as a [num_observations, num_classes] matrix, regardless of lightgbm version
 reshape_lightgbm_multiclass_preds <- function(preds, num_rows) {
-    n_preds_per_case <- length(preds) / num_rows


More space deletions here that are unrelated to the PR.

simonpcouch · 2023-12-22T16:39:45Z

-      p <- stats::predict(object$fit, prepare_df_lgbm(new_data), type = "raw", ...)
+    p <- stats::predict(object$fit, prepare_df_lgbm(new_data), type = "raw", ...)
  } else {
-      p <- stats::predict(object$fit, prepare_df_lgbm(new_data), rawscore = TRUE, ...)


Here as well.

simonpcouch · 2023-12-22T16:40:48Z

This file, lightgbm.new.md, is an intermediate file from testthat. To merge its changes into the existing test-lightgbm.R, use testthat::snapshot_accept().

simonpcouch · 2024-04-07T19:07:47Z

Thanks again for your work here. Given the lack of movement on this PR, I'm going to go ahead and close. :)

joranE · 2024-04-08T13:29:01Z

I have found a need for this feature recently, so I may try to resurrect this pull request and finish cleaning it up sometime this week. Anything I should know besides your comments above, @simonpcouch ?

simonpcouch · 2024-04-08T13:30:48Z

That'd be wonderful, @joranE! Conceptually, the implementation here looks solid—following the current review comments would put this PR in a good spot.

p-schaefer · 2024-04-08T13:38:57Z

Sorry, this fell off my radar. I can finish addressing the comments next week, if that works for you. Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Simon P. Couch ***@***.***> Sent: Monday, April 8, 2024 9:31:10 AM To: tidymodels/bonsai ***@***.***> Cc: Patrick Schaefer ***@***.***>; Author ***@***.***> Subject: Re: [tidymodels/bonsai] added support for case_weights (PR #72) That'd be wonderful, @joranE<https://github.com/joranE>! Conceptually, the implementation here looks solid—following the current review comments would put this PR in a good spot. — Reply to this email directly, view it on GitHub<#72 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACYA7SXU5VRFVBVDIZ322P3Y4KLZ5AVCNFSM6AAAAABAUFJUD6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBSG43DONJYGY>. You are receiving this because you authored the thread.Message ID: ***@***.***>

joranE · 2024-04-08T15:14:18Z

@p-schaefer Sounds good, I'll let you tackle it! If you find yourself pulled away from it again for any reason, just let me know.

p-schaefer · 2024-04-09T00:43:40Z

I've fixed the spacing issues, and dealt with the intermediate file from testthat. Should I submit a new PR?

simonpcouch · 2024-04-09T13:36:11Z

You're welcome to push to the same branch! I'll reopen this one.

p-schaefer · 2024-04-09T16:09:13Z

Sorry, forgot to rerun the tests and checks. Looks good to me now, let me know if there is anything else.

joranE · 2024-04-09T17:51:47Z

Thanks for closing the loop on this @p-schaefer! Now I just need to figure out if there's an easy way to tweak this interface to allow for the use of the linear_tree parameter for piecewise linear trees, which "naturally" needs to be passed to lgb.Dataset() as well, instead of lgb.train(). 🙄

p-schaefer · 2024-04-09T18:32:32Z

No problem. It might be worth opening a new issue to discuss how to deal with linear_tree (if you want some help). I think we can probably come up with a suitably tidy workflow for dealing with arguments that should be passed to lgb.Dataset() vs lgb.train(). I've been thinking of playing around with linear_tree as well.

joranE · 2024-04-09T18:55:03Z

I added a new issue here.

simonpcouch · 2024-04-09T19:46:01Z

Test failures are unrelated, feel free to ignore. :)

p-schaefer · 2024-04-12T12:31:47Z

@simonpcouch Was there anything else you need from me?

simonpcouch · 2024-05-08T16:44:02Z

Nice work, @p-schaefer! :)

added support for case_weights

4aafd36

p-schaefer mentioned this pull request Dec 14, 2023

add support for case weights with lightgbm engine #65

Closed

simonpcouch reviewed Dec 15, 2023

View reviewed changes

Patrick Schaefer added 4 commits December 15, 2023 11:57

added support for providing a validation_groups parameter - A charact…

0d5734b

…er vector specifying one or more variables in `x`used for grouping observations with the same value to either the training or validation set.

reverted addition of validation groups and moved development to separ…

b1c5a5d

…ate branch

cleaned syntax

7df6250

cleaned testing

2697a10

simonpcouch reviewed Dec 22, 2023

View reviewed changes

simonpcouch closed this Apr 7, 2024

restored original spaces

ca0dadf

simonpcouch reopened this Apr 9, 2024

updated tests

82b0c94

simonpcouch reviewed Apr 9, 2024

View reviewed changes

Comment thread tests/testthat/test-lightgbm.R Outdated

used tidy syntax in tests

2579170

simonpcouch added 3 commits May 8, 2024 09:06

merge upstream

16d8c0e

test that fits are exactly equal

792c10e

note case weight support with lightgbm [no ci]

392bd0a

simonpcouch merged commit f40d55a into tidymodels:main May 8, 2024

	train_lightgbm <- function(x, y,weights=NULL, max_depth = -1, num_iterations = 100, learning_rate = 0.1,
	train_lightgbm <- function(x, y, weights = NULL, max_depth = -1, num_iterations = 100, learning_rate = 0.1,

Conversation

p-schaefer commented Dec 14, 2023

Uh oh!

simonpcouch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

p-schaefer commented Dec 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpcouch commented Apr 7, 2024

Uh oh!

joranE commented Apr 8, 2024

Uh oh!

simonpcouch commented Apr 8, 2024

Uh oh!

p-schaefer commented Apr 8, 2024 via email

Uh oh!

joranE commented Apr 8, 2024

Uh oh!

p-schaefer commented Apr 9, 2024

Uh oh!

simonpcouch commented Apr 9, 2024

Uh oh!

p-schaefer commented Apr 9, 2024

Uh oh!

joranE commented Apr 9, 2024

Uh oh!

p-schaefer commented Apr 9, 2024

Uh oh!

joranE commented Apr 9, 2024

Uh oh!

Uh oh!

simonpcouch commented Apr 9, 2024

Uh oh!

p-schaefer commented Apr 12, 2024

Uh oh!

simonpcouch commented May 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants