fix: correct Tweedie deviance formula for power not in {0, 1} - #270
Open
AlejandroCoronadoN wants to merge 3 commits into
Open
fix: correct Tweedie deviance formula for power not in {0, 1}#270AlejandroCoronadoN wants to merge 3 commits into
AlejandroCoronadoN wants to merge 3 commits into
Conversation
The factor of 2 was applied to only the first deviance term for a general power and for the power == 2 (gamma) case, so the other terms were left un-doubled. This produced wrong values, including negative "deviances" for 1 < power < 2. Distribute the 2 over all three terms so the result matches scikit-learn's mean_tweedie_deviance for every power. The test reference for power == 2 carried the same mistake, so it is corrected too, and general-power cases (1.5 and 2.5) are added to the loss comparison test.
|
|
Contributor
|
Hi @AlejandroCoronadoN thanks for this fix! I just updated the documentation to show the correct formula. Can you sign the license so we can merge your PR? We would like to make a release asap, including your fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
For a general Tweedie
power(and for thepower == 2gamma case), the factorof 2 was only applied to the first deviance term:
The unit Tweedie deviance doubles all three terms. Because of this, the result
was wrong for every power other than 0 and 1, and even negative for
1 < power < 2(a deviance can never be negative).The bug went unnoticed because the loss comparison test only covered powers 0,
1 and 2, and its own reference for
power == 2repeated the same mistake.Fix
Distribute the
2 *over all three terms in both the general-power branch andthe
power == 2branch.Verification
After the fix,
tweedie_deviancematchessklearn.metrics.mean_tweedie_deviancefor powers 0, 1, 1.2, 1.5, 1.8, 2.0, 2.5 and 3.0 (max abs diff < 1e-6). The test
reference for
power == 2is corrected to the same formula, and general-powercases (1.5 and 2.5) are added to
test_loss, which fail without the code changeand pass with it. The full
test_losses.pysuite (106 tests, pandas and polars)passes.