[BUG]: missing multiplication in `_modifiedHuberLoss` implementation in `ml/incr/binary-classification`

[nakul-krishnakumar]

Description

I was going through the wikipedia pages as well as other library implementations of loss functions to implement standalone loss functions and their gradients under ml/base/loss/float64/*.
I noticed a possible bug in _modifiedHuberLoss function implemented under ml/incr/binary-classification

Current implementation:

setReadOnly( Model.prototype, '_modifiedHuberLoss', function modifiedHuber( x, y ) {
	var eta;
	var d;

	eta = this[ this._learningRateMethod ]();
	this._regularize( eta );

	d = y * this._dot( x.data, x.strides[ 0 ], x.offset );
	if ( d < -1.0 ) {
		this._add( x, 4.0*eta*y );
	} else {
		this._add( x, eta*( y-(d*y) ) );
	}
	return this;
});

Link

I believe in the else branch, it should be this._add( x, eta*2*( y-(d*y) ) ) instead of this._add( x, eta*( y-(d*y) ) ). That is, it seems like it is missing a multiplication with 2 here.

References:
Wikipedia
sklearn

cc: @Planeshifter

Related Issues

No response

Questions

No.

Demo

No response

Reproduction

• a
• b
• c

Expected Results

$$\frac{\partial \ell}{\partial p} = \begin{cases} -4y & \text{if } yp < -1 \\\ -2y(1 - yp) & \text{if } -1 \le yp \le 1 \\\ 0 & \text{if } yp > 1 \end{cases}$$

Actual Results

Version

No response

Environments

N/A

Browser Version

No response

Node.js / npm Version

No response

Platform

No response

Checklist

• Read and understood the Code of Conduct.
• Searched for existing issues and pull requests.

[A-S-Manoj]

Confirmed — checked against sklearn's _sgd_fast.pyx, which has 2.0 * (1.0 - z) * -y for the quadratic branch. That 2 comes from differentiating (1-yf)², so the update should be 2η(y−dy), matching the issue.
I'd be happy to open a PR for this.

[kgryte]

@Planeshifter Perhaps you had a rationale for omitting the 2 factor?

[nakul-krishnakumar]

I can see a similar convention of omitting the 2 factor for squared-hinge-loss too in ml/incr/binary-classification. Ref

[Planeshifter]

@nakul-krishnakumar I believe you are correct and that the 2 factor is indeed missing.

Looking at the gradient equation, it seems we are also missing a yp ≥ 1 clamp in the modified Huber loss implementation. _squaredHingeLoss does clamp correctly if d < 1.0.

For reference, here is scikit-learn:

    cdef double cy_gradient(self, double y, double p) noexcept nogil:
        cdef double z = p * y
        if z >= 1.0:
            return 0.0
        elif z >= -1.0:
            return 2.0 * (1.0 - z) * -y
        else:
            return -4.0 * y

(https://github.com/scikit-learn/scikit-learn/blob/fe2edb3cdbd75ae4e662fda67dcb19277258792b/sklearn/linear_model/_sgd_fast.pyx.tp#L140-L149)

Please feel free to open a fix PR.

[nakul-krishnakumar]

Thank you @Planeshifter, I will open a fixing PR shortly.