-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom multiclass objectives in R #4996
Conversation
…#2776 with this change the https://github.com/pmontman/fforma package works fine.
Codecov Report
@@ Coverage Diff @@
## master #4996 +/- ##
==========================================
+ Coverage 81.66% 83.76% +2.10%
==========================================
Files 11 11
Lines 2389 2409 +20
==========================================
+ Hits 1951 2018 +67
+ Misses 438 391 -47
Continue to review full report at Codecov.
|
Could you please help taking a look @pommedeterresautee ? |
Tks, I check today or tomorrow. |
@Someone894 Hi, I have made few checks and it seems to not break simple cases (I was worried about the reshaping). |
The |
Thank you a lot for these information. xgboost/R-package/R/xgb.train.R Line 331 in 5e97de6
Its purpose is to apply the objective to the predictions of the model being built and compute the loss. Line 149 in 9536a7e
This PR modifies the predictions by setting outputmargin to True , basically the predictions are now the margin directly computed by XGBoost (the original untransformed predictions).My feeling is that in several cases this will have unwanted effect on the objective, and the quality of the model. Would it be possible for you to contact @pmontman to see if there are reasons why such change would be Ok? Otherwise, I am thinking that another approach may be more appropriate. Regarding the reshape thing, it should be easy to perform in the objective itself. |
Thanks @jeffzi . I understand the purpose of the change, my question is how this change may not have any impact on other use cases? because here we are wondering if it makes sense to apply the change to the main library. I may miss something. And may be there is something else to do to avoid to impact all use cases? |
I tried to reach @pmontman to also have a look at this topic, but nothing happened, as you can see. |
@Someone894 @pommedeterresautee It makes sense to me to use raw prediction here, as objective function should handle this transformation itself. WDYT? |
It makes sense to me to use raw prediction, since it reflects what happens when no custom objective is given. Lines 145 to 151 in ad4a1c7
Lines 561 to 567 in ad4a1c7
|
Hi, I've encountered this problem as well. Will there be an update soon? Thanks. |
I incline to merge this PR(or new one with tests). @hcho3 WDYT? |
Added workaround described in #2776
with this change the https://github.com/pmontman/fforma package works fine.