Skip to content
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.

Sign up for GitHub

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

fix calculation of weighted gamma loss (fixes #4174) #4283

Merged
merged 8 commits into from
May 21, 2021
Merged

fix calculation of weighted gamma loss (fixes #4174) #4283

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9b7ae72
fixed weighted gamma obj
mayer79 May 13, 2021
9d198ff
added unit tests
mayer79 May 14, 2021
75d7ac9
fixing linter errors
mayer79 May 14, 2021
c8d0001
another linter
mayer79 May 14, 2021
52e596d
set seed
mayer79 May 15, 2021
411efd6
Merge branch 'microsoft:master' into weighted_gamma
mayer79 May 15, 2021
3ea27ba
fix linter (integer seed)
mayer79 May 15, 2021
1e60df1
Merge branch 'master' into weighted_gamma
mayer79 May 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/objective/regression_objective.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,7 +695,7 @@ class RegressionGammaLoss : public RegressionPoissonLoss {
} else {
#pragma omp parallel for schedule(static)
for (data_size_t i = 0; i < num_data_; ++i) {
gradients[i] = static_cast<score_t>(1.0 - label_[i] / std::exp(score[i]) * weights_[i]);
gradients[i] = static_cast<score_t>((1.0 - label_[i] / std::exp(score[i])) * weights_[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
gradients[i] = static_cast<score_t>((1.0 - label_[i] / std::exp(score[i])) * weights_[i]);
gradients[i] = static_cast<score_t>((1.0 - label_[i] * std::exp(-score[i])) * weights_[i]);

Could be a tiny little bit faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorentzenchr
Regarding timing: the same pattern (i.e. y/exp(p) instead of y*exp(-p) appears multiple time in the same file. My suggestion would be to open a new issue/PR that would change this consistently, along with 1-2 experiments on timings.

Regarding unit tests: I added some in a new R script. On the current master, it fails. With the gamma weight fix it passes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the exp dominates timing by far.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should make the formula in the same file consistent. If we want to change division into multiplication, we should modify all these

LightGBM/src/objective/regression_objective.hpp

Lines 689 to 700 in adf36d7

if (weights_ == nullptr) {
#pragma omp parallel for schedule(static)
for (data_size_t i = 0; i < num_data_; ++i) {
gradients[i] = static_cast<score_t>(1.0 - label_[i] / std::exp(score[i]));
hessians[i] = static_cast<score_t>(label_[i] / std::exp(score[i]));
}
} else {
#pragma omp parallel for schedule(static)
for (data_size_t i = 0; i < num_data_; ++i) {
gradients[i] = static_cast<score_t>(1.0 - label_[i] / std::exp(score[i]) * weights_[i]);
hessians[i] = static_cast<score_t>(label_[i] / std::exp(score[i]) * weights_[i]);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #4289.

hessians[i] = static_cast<score_t>(label_[i] / std::exp(score[i]) * weights_[i]);
}
}
Expand Down