fix calculation of weighted gamma loss (fixes #4174)

[mayer79]

Fixes #4174

[jameslamb]

Thanks for this! I've edited the pull request title to be more descriptive. Pull request titles become changelog entries in this project (please see https://github.com/microsoft/LightGBM/releases/tag/v3.2.1).

[jameslamb]

I don't think the CUDA build failures are related to these changes. All of the Dask tests are failing there with the following error

>       from distributed.protocol.core import dumps_msgpack
E       ImportError: cannot import name 'dumps_msgpack' from 'distributed.protocol.core' (/root/miniconda/envs/test-env/lib/python3.7/site-packages/distributed/protocol/core.py)

I can look into this in a few hours. I suspect that this is a result of some new version of a dependency being released, will have to check them later to see.

[lorentzenchr]

This fixed the bug. What about a unit test for this?

[lorentzenchr]

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.

[mayer79]

@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.

[lorentzenchr]

the exp dominates timing by far.

[shiyu1994]

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]);
	}

[lorentzenchr]

Done in #4289.

[jameslamb]

Ok @mayer79 , now that we've merged #4288, if you update to the latest master I think the failing checks should pass. Sorry for the inconvenience!

[jameslamb]

Tests and changes look good to me! But another maintainer who's more familiar with gamma loss should probably review as well, so I'm just leaving a "comment" review.

[jameslamb]

This test looks good to me, thank you! This might be the first unit test we've added that covers weighted training at all in the R package, so really appreciate it.

It's ok with me for this to live in a new test file as you've set it up. test_basic.R (where most of the other lgb.train() tests live) has gotten kind of big, and there are already uses of lgb.train() outside of that file in their own files (such as https://github.com/microsoft/LightGBM/blob/c629cb0b6bd2830894b710cbd4d8241b82ac3105/R-package/tests/testthat/test_learning_to_rank.R or https://github.com/microsoft/LightGBM/blob/c629cb0b6bd2830894b710cbd4d8241b82ac3105/R-package/tests/testthat/test_custom_objective.R).

[lorentzenchr]

@shiyu1994 After having reviewed #4289, maybe you'd like to have another look here as it fixes an actual bug.

[shiyu1994]

The changes LGTM.

[github-actions]

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.