Fix platt scaling #4166

durovo · 2018-02-09T18:48:15Z

This PR fixes platt scaling and adds new classes SigmoidCalibration and Calibration. Previously, fit_sigmoid was only using the predictions to fit parameters which is incorrect. SigmoidCalibration has been added so that score_to_probabilites from CBinaryLabels can be removed. This will make the x-validated calibration API cleaner (#4009 ). The old fit_sigmoid and scores_to_probabilities have still not be removed since they are still needed in multiclass calibration. It would probably be safe to remove them after fixing #4164

durovo · 2018-02-10T12:17:32Z

The SIGSEV on SGObject seems to be due to some code in upstream develop.

vigsterkr · 2018-02-10T12:53:32Z

src/shogun/evaluation/Calibration.cpp

+
+using namespace shogun;
+
+CCalibration::CCalibration()


this seems to be an abstract class, as nothing is implemented.
why dont you just drop the implementation and specify all the virtual methods pure virtual?

I was thinking of adding common fit and calibrate methods that would call fit_binary, fit_multiclass etc as required. Yes, there doesn't seem to be any point in the implementation. I'll just add those in the header file itself.

vigsterkr · 2018-02-10T12:54:36Z

src/shogun/evaluation/SigmoidCalibration.h

+
+	private:
+		/** Array to store sigmoid parameters for each class. In case of binary labels, only one pair of parameters are stored. */
+		CStatistics::SigmoidParamters* m_sigmoid_parameters;


wouldn't storing this in an std::vector would be better?

or in worst case DynamicObjectArray, for assuring ref counting

I will replace this with std::vector... DynamicObjectArray seems to support only CSGObjects, it can't hold structs like CStatistics::SigmoidParamters. Please correct me if I'm wrong about that :)

vigsterkr · 2018-02-10T12:56:27Z

src/shogun/evaluation/SigmoidCalibration.cpp

+	index_t num_classes =
+	    predictions->get_num_classes();
+
+	SG_FREE(m_sigmoid_parameters);


as said i would rather use something more high level than this

vigsterkr · 2018-02-10T12:59:46Z

src/shogun/mathematics/Statistics.cpp

+	int32_t prior1=0;
+	SG_SDEBUG("counting number of positive and negative labels\n")
+	{
+		for (index_t i=0; i<scores.vlen; ++i)


isn't it enough to count the one of them as the other can be simply deducted?
i would use something like std::count_if or std::transform

Yes, counting one of them would be enough. I using std::count_if would indeed be neat.

vigsterkr · 2018-02-10T13:00:43Z

src/shogun/mathematics/Statistics.cpp

+
+	/* parameter setting */
+	/* maximum number of iterations */
+	index_t maxiter=100;


all these parameters should be set somewhere else... as you want to be able to control them without actually changing the code... i.e. there should be a getter/setter for them

I am not the one who originally implemented fit_sigmoid, I merely made a few corrections in it. However, the code can certainly be improved. I'm on it.

vigsterkr · 2018-02-10T13:01:00Z

src/shogun/mathematics/Statistics.cpp

+	index_t length=prior1+prior0;
+
+	SGVector<float64_t> t(length);
+	for (index_t i=0; i<length; ++i)


same as above... too vanilla implementation

yes, can't this be done a bit more modern?

you can either use std::transform or at least use the fact that SGVector has iterator so it can actually do a range-loop

Will do, thanks for the suggestion

@vigsterkr I opted to use std::transform

durovo · 2018-02-10T13:52:52Z

Thanks for the review! I will try and make all the requested changes.

durovo · 2018-02-10T14:32:45Z

@vigsterkr While making all the methods pure virtual would be nice, wouldn't we lose the informative messages generated by SG_NOTIMPLEMENTED? Suppose, the implementation class only want to use fit_binary wouldn't it be useful to have the message generated by SG_NOTIMPLEMENTED when fit_multiclass is called?

vigsterkr · 2018-02-11T19:39:09Z

src/shogun/evaluation/SigmoidCalibration.h

+	 	/** Set maximum number of iterations
+	 	* @param maxiter maximum number of iterations
+	 	*/
+	 	virtual void set_maxiter(index_t maxiter);


why are these all virtual? you will have a calibration method that is based on Sigmoid?

Probably not. Indeed there is no need to make them virtual.

vigsterkr · 2018-02-11T19:39:28Z

src/shogun/evaluation/SigmoidCalibration.cpp

@@ -42,28 +42,75 @@

 using namespace shogun;

-CSigmoidCalibration::CSigmoidCalibration() : CCalibration()
+CSigmoidCalibration::CSigmoidCalibration()


it should call the ctor of CCalibration

Yes, my bad

vigsterkr · 2018-02-11T19:39:51Z

src/shogun/evaluation/Calibration.h

@@ -64,26 +70,44 @@ namespace shogun
 		* @param targets The true labels corresponding to the predictions
 		* @return boolean indicating whether the calibration was succesful
 		**/
-		virtual bool fit_binary(CBinaryLabels* predictions, CBinaryLabels* targets);
+		virtual bool fit_binary(CBinaryLabels* predictions, CBinaryLabels* targets)


i still dont get why do we need default implementation with error

I'm not sure about making all methods pure virtual... suppose a calibration method does not support multiclass calibration, then the implementation will have to derive fit_multiclass and give error message manually. Would that be fine?

vigsterkr · 2018-02-11T19:41:45Z

src/shogun/evaluation/SigmoidCalibration.cpp

+	m_minstep = 1E-10;
+	m_sigma = 1E-12;
+	m_epsilon = 1E-5;
+


m_sigmoid_parameters is not registered... that'll make it impossible to clone this class. since currently registering std::vector is not possible you would need to use DynArray :(

Thanks for telling me about DynArray, I was looking for the standard shogun way of storing arrays of non-CSGObjects.

I have opted to use two SGVectors for now

Think that should be OK as a workaround for now.

karlnapf · 2018-02-12T10:46:39Z

src/shogun/evaluation/Calibration.h

+ * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND


could you use the new (shorter) bsd header? see some other files

This one? /*

This software is distributed under BSD 3-clause license (see LICENSE file).

Authors:
*/

karlnapf · 2018-02-19T20:26:22Z

tests/unit/evaluation/SigmoidCalibration_unittest.cc

+ * Written (w) 2012 - 2013 Heiko Strathmann
+ * Written (w) 2018 Dhruv Arya
+ * All rights reserved.
+ *


we can also use the small license here

karlnapf · 2018-02-19T20:26:40Z

src/shogun/mathematics/Statistics.h

+		index_t maxiter = 100, float64_t minstep = 1E-10,
+		float64_t sigma = 1E-12, float64_t eps = 1E-5);
+
+	/** TODO: Remove this method after fixing MuticlassCalibration


pls no todo in patches.

karlnapf · 2018-02-19T20:27:11Z

src/shogun/mathematics/Statistics.cpp

+
+	if (it>=maxiter-1)
+	{
+		SG_SWARNING("CStatistics::fit_sigmoid(): reaching maximal iterations,"


we dont do this ClassName::MethodName(): anymore. Just the message

This code is mostly taken from the previous fit_sigmoid. Do you recommend changing all the messages?

yeah let's modernize a bit :)

karlnapf · 2018-02-19T20:28:03Z

src/shogun/mathematics/Statistics.cpp

+	/* initial Point and Initial Fun Value */
+	/* result parameters of sigmoid */
+	float64_t a=0;
+	float64_t b=CMath::log((prior0+1.0)/(prior1+1.0));


where is this code coming from?

It's all from the old fit_sigmoid

karlnapf · 2018-02-19T20:28:24Z

src/shogun/mathematics/Statistics.cpp

+
+	for (index_t i=0; i<length; ++i)
+	{
+		float64_t fApB=scores[i]*a+b;


if this code is copy pasted, fine. Otherwise, can we rename this variable? :)

It is mostly copy-pasted. Should I rename it?

maybe leave it so it is clear where the code is coming from

karlnapf

What is the state of this? :)

durovo · 2018-02-20T13:58:13Z

@karlnapf I have added a test for multiclass calibration.

karlnapf · 2018-02-20T16:07:36Z

src/shogun/evaluation/Calibration.h

+
+namespace shogun
+{
+	/** @brief Base class for all calibration methods. Call fit to 


Can you tell doxygen that "fit" is a method here?

karlnapf · 2018-02-20T16:08:00Z

src/shogun/evaluation/Calibration.h

+		/** Fit calibration parameters for binary labels.
+		* @param predictions The predictions outputted by the machine
+		* @param targets The true labels corresponding to the predictions
+		* @return boolean indicating whether the calibration was succesful


minor: no need to state the type in return, c++ tell you that already

Understood.

karlnapf · 2018-02-20T16:10:06Z

src/shogun/labels/MulticlassLabels.cpp

+{
+	REQUIRE((m_multiclass_confidences.num_rows != 0) && 
+		(m_multiclass_confidences.num_cols != 0),
+		"Call allocate_confidences_for() before fetching confidences");


missing newline

I woudl also suggest to change the error msg
"Empty confidences, which need to be allocated before fetching.\n")

Okay, that looks much better :)

karlnapf · 2018-02-20T16:12:00Z

tests/unit/evaluation/SigmoidCalibration_unittest.cc

+ //       [0.31003506, 0.60319518, 0.08676976],
+ //       [0.26630134, 0.01476002, 0.71893864],
+ //       [0.72470815, 0.20387599, 0.07141586],
+ //       [0.074445  , 0.80897488, 0.11658012],


could you remove those?

karlnapf · 2018-02-20T16:12:33Z

tests/unit/evaluation/SigmoidCalibration_unittest.cc

+	   0.75674645, 0.20144443, 0.04180912,
+       0.82689269, 0.05965391, 0.11345341,
+       0.81409222, 0.12715657, 0.05875121,
+       0.48765218, 0.38500051, 0.12734731,


where do the numbers in the tests come from?

I trained a linear SVC in sklearn, this is the decision function of that SVC. The expected output is the probabilities of the calibrated SVC.

thats good! Could maybe be added as a comment in there. But nevermind for now

karlnapf

Getting there!
I think one more iteration and we are good :)

durovo · 2018-02-22T13:22:47Z

@karlnapf I have made all the requested changes. Code style fix should be done after this has been approved right?

karlnapf · 2018-02-22T16:43:47Z

examples/undocumented/libshogun/classifier_libsvm_probabilities.cpp

@@ -73,16 +75,19 @@ int main(int argc, char** argv)
    using the method described in Lin, H., Lin, C., and Weng,  R. (2007). A note
    on Platt's probabilistic outputs for support vector machines.
    See BinaryLabels documentation for details*/
-    out_labels->scores_to_probabilities();
+    CSigmoidCalibration* sigmoid_calibration = new CSigmoidCalibration();


is this deleted anywhere? SG_REF?

I missed this, sorry for that.

karlnapf · 2018-02-22T16:44:10Z

examples/undocumented/libshogun/labels_binary_fit_sigmoid.cpp


 using namespace shogun;

 void test_sigmoid_fitting()
 {
 	CBinaryLabels* labels=new CBinaryLabels(10);
+	CBinaryLabels* predictions=new CBinaryLabels(10);


deleted somewhere?

karlnapf · 2018-02-22T16:45:29Z

examples/undocumented/libshogun/labels_binary_fit_sigmoid.cpp

+	CSigmoidCalibration* sigmoid_calibration = new CSigmoidCalibration();
+	sigmoid_calibration->fit_binary(predictions, labels);
+	CBinaryLabels* calibrated_labels = sigmoid_calibration->calibrate_binary(predictions);
+	calibrated_labels->get_values().display_vector("probabilities");


why display?

I'm not sure myself. It was in the original code

shogun/examples/undocumented/libshogun/labels_binary_fit_sigmoid.cpp

Line 22 in 297f9a0

labels->get_values().display_vector("probabilities");

I see. Not your fault of course. Would still be better removed :)
In fact, what would be best was to remove the example altogether (another PR) and have a meta example for that

karlnapf · 2018-02-22T16:51:14Z

src/shogun/evaluation/SigmoidCalibration.cpp

+	params.b = m_sigmoid_bs[0];
+
+	/** Convert predictions to probabilties. */
+	auto values = calibrate_values(predictions->get_values(), params);


rather than the comment, I would name the variable probabilities and then you dont need it. Minor of course

That would look much better indeed

karlnapf · 2018-02-22T16:51:36Z

src/shogun/evaluation/SigmoidCalibration.cpp

+	m_sigmoid_as.resize_vector(num_classes);
+	m_sigmoid_bs.resize_vector(num_classes);
+
+	 /** Fit and store parameters for for each class seperately. */


no need for this comment

karlnapf · 2018-02-22T16:52:39Z

src/shogun/evaluation/SigmoidCalibration.cpp

+	index_t num_classes = predictions->get_num_classes();
+	index_t num_samples = predictions->get_num_labels();
+
+	/** Matrix to temporarily store the probabilities. */


I would never write the type of a variable in a comment, but minor

Yeah, it seems does seem very redundant. Will change this.

karlnapf · 2018-02-22T16:53:09Z

src/shogun/evaluation/SigmoidCalibration.cpp

+		float64_t sum = SGVector<float64_t>::sum(values);
+
+		/** All classes have equal probability when sum is zero */
+		if (sum == 0)


good catch!

karlnapf · 2018-02-22T16:54:08Z

src/shogun/evaluation/SigmoidCalibration.h

+namespace shogun
+{
+	/** @brief Calibrates labels based on Platt Scaling [1]. Note that first calibration parameters need to be fitted by 
+	* calling fit_binary() or fit_multiclass(). Then call calibrate on calibrate_binary() or calibrate_multiclass() on 


what will happen if a user forgets to call fit first?

The length of the parameter vector is checked in calibrate now and an appropriate message is displayed.

karlnapf · 2018-02-22T16:54:34Z

src/shogun/evaluation/SigmoidCalibration.h

+		SGVector<float64_t> calibrate_values(SGVector<float64_t> values, CStatistics::SigmoidParamters params);
+
+	private:
+		/** Vector to store parameter A of sigmoid for each class. In case of binary labels, only one pair of parameters are stored. */


no type in the comments or doxygen, it is clear from the code

Should it the first line be something like: "Stores parameter A of sigmoid for each class"?

Yes, but that is really me nitpicking. Just for the future

karlnapf · 2018-02-22T16:55:15Z

src/shogun/mathematics/Statistics.cpp

+{
+	SG_SDEBUG("entering CStatistics::fit_sigmoid()\n")
+
+	REQUIRE(scores.vector, "Specify scores vector!\n");


"Provided scores are empty.\n"

Understood.

karlnapf · 2018-02-22T16:55:19Z

src/shogun/mathematics/Statistics.cpp

+	index_t maxiter, float64_t minstep,
+	float64_t sigma, float64_t epsilon)
+{
+	SG_SDEBUG("entering CStatistics::fit_sigmoid()\n")


karlnapf · 2018-02-22T16:56:19Z

tests/unit/evaluation/SigmoidCalibration_unittest.cc

+	EXPECT_EQ(calibrated, true);
+
+	auto calibrated_labels = sigmoid_calibration->calibrate_binary(predictions);
+


why all the empty lines?

Not required? Will remove them.

karlnapf

I think this is good to go very soon
I put some minor comments in if you wanna do another iteration, but happy to merge (modulo CI) otherwise
@vigsterkr what do you think?

vigsterkr · 2018-02-22T16:59:18Z

src/shogun/mathematics/Statistics.cpp

+	index_t length=prior1+prior0;
+
+	SGVector<float64_t> t(length);
+	for (index_t i=0; i<length; ++i)


you can either use std::transform or at least use the fact that SGVector has iterator so it can actually do a range-loop

durovo · 2018-02-23T14:55:28Z

@vigsterkr @karlnapf I have made the requested changes. Should I fix the style, rebase and push?

karlnapf · 2018-02-23T15:35:54Z

Yes! style fix and let's merge. Nice work!

karlnapf · 2018-02-27T08:51:07Z

🎉

vigsterkr requested changes Feb 10, 2018

View reviewed changes

durovo mentioned this pull request Feb 11, 2018

Implement cross-validated calibration #3851 #4009

Closed

vigsterkr requested changes Feb 11, 2018

View reviewed changes

karlnapf reviewed Feb 12, 2018

View reviewed changes

durovo force-pushed the feature/platt branch 2 times, most recently from 6f3b546 to 8e0e97b Compare February 18, 2018 11:31

karlnapf reviewed Feb 19, 2018

View reviewed changes

durovo force-pushed the feature/platt branch 2 times, most recently from 648c371 to c769d80 Compare February 20, 2018 13:55

karlnapf reviewed Feb 20, 2018

View reviewed changes

karlnapf requested changes Feb 20, 2018

View reviewed changes

karlnapf reviewed Feb 22, 2018

View reviewed changes

vigsterkr requested changes Feb 22, 2018

View reviewed changes

vigsterkr approved these changes Feb 23, 2018

View reviewed changes

karlnapf approved these changes Feb 23, 2018

View reviewed changes

durovo force-pushed the feature/platt branch from 64c86ac to 56cde39 Compare February 23, 2018 16:07

fix platt scaling

d679103

durovo force-pushed the feature/platt branch from 56cde39 to d679103 Compare February 26, 2018 17:59

vigsterkr changed the title ~~WIP: Fix platt scaling~~ Fix platt scaling Feb 26, 2018

vigsterkr merged commit 0a2c468 into shogun-toolbox:develop Feb 26, 2018

		EXPECT_EQ(calibrated, true);

		auto calibrated_labels = sigmoid_calibration->calibrate_binary(predictions);

Fix platt scaling #4166

Fix platt scaling #4166

Conversation

durovo commented Feb 9, 2018

durovo commented Feb 10, 2018

Choose a reason for hiding this comment

durovo Feb 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durovo Feb 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durovo commented Feb 10, 2018

durovo commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf Feb 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durovo Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

durovo commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durovo Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

durovo commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durovo Feb 10, 2018 •

edited

durovo Feb 10, 2018 •

edited

karlnapf Feb 22, 2018 •

edited

durovo Feb 20, 2018 •

edited

durovo Feb 20, 2018 •

edited