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.

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

WIP: Make sure Done() is called in cleanup if it hasn't been called #4817

Closed
wants to merge 3 commits into from
Closed

WIP: Make sure Done() is called in cleanup if it hasn't been called #4817

wants to merge 3 commits into from

Conversation

harishsk
Copy link
Contributor

@harishsk harishsk commented Feb 8, 2020

This makes sure call stack is collected for intermediate failures.

@@ -455,7 +455,7 @@ private protected override VBuffer<float> InitializeWeightsFromPredictor(IPredic
CurrentWeights.CopyTo(ref weights, 1, CurrentWeights.Length - 1);
return new ParameterMixingCalibratedModelParameters<LinearBinaryModelParameters, PlattCalibrator>(Host,
new LinearBinaryModelParameters(Host, in weights, bias, _stats),
new PlattCalibrator(Host, -1, 0));
new PlattCalibrator(Host, 1, 0));

Choose a reason for hiding this comment

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

It seems that the bug is actually in the PlattCalibrator itself:

While in the comment it says this:

    /// The Platt calibrator calculates the probability following:
    /// P(x) = 1 / (1 + exp(-<see cref="PlattCalibrator.Slope"/> * x + <see cref="PlattCalibrator.Offset"/>)

The actual calculation does this:

        public float PredictProbability(float output)
        {
            if (float.IsNaN(output))
                return output;
            return PredictProbability(output, Slope, Offset);
        }

        internal static float PredictProbability(float output, Double a, Double b)
        {
            return (float)(1 / (1 + Math.Exp(a * output + b)));
        }

The value here should be changed to 1 only if we change the computation in PlattCalibrator. (looking at the baselines, the computation without the change looks better: label 1 examples give higher probabilities, and label 0 examples give lower probabilities.)

Copy link

@yaeldekel yaeldekel left a comment

Choose a reason for hiding this comment

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

🕐

@frank-dong-ms-zz
Copy link
Contributor

I remember you said this PR is still in progress, right?
If so, you can use draft PR before it is ready

@harishsk harishsk changed the title Make sure Done() is called in cleanup if it hasn't been called WIP: Make sure Done() is called in cleanup if it hasn't been called Feb 10, 2020
@harishsk
Copy link
Contributor Author

I forgot to add a draft flag earlier. I have changed the title to include WIP now. I will most likely close this PR and reopen a new one.


In reply to: 584371281 [](ancestors = 584371281)

@harishsk
Copy link
Contributor Author

harishsk commented Mar 2, 2020

This PR is superseded by #4823 . Closing this one.

@harishsk harishsk closed this Mar 2, 2020
@harishsk harishsk deleted the testCleanup branch April 21, 2020 23:59
@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants