fix: correct modified Huber loss gradient in `ml/incr/binary-classification` by Abhist17 · Pull Request #15317 · stdlib-js/stdlib

Abhist17 · 2026-09-18T06:48:33Z

Description

Fixes #13111. The else branch of _modifiedHuberLoss in ml/incr/binary-classification's Model was missing a factor of 2 in the "quadratically smoothed" region, and was being applied unconditionally for d = y*f(x) < 1 — including the d >= 1 region, where the modified Huber loss (and therefore its gradient) is exactly zero.

This goes slightly beyond what #13111 literally flagged (just the missing 2): I independently re-derived the three-region gradient against the cited references before changing anything, since applying only the factor-of-2 fix without also bounding the branch at d < 1.0 would still be wrong for d >= 1.

Per the modified Huber loss definition (Wikipedia, "Variant for classification"; scikit-learn's _sgd_fast.pyx.tp ModifiedHuber.dloss), the gradient of the loss w.r.t. the decision value p = y*f(x) is:

p >= 1: 0
-1 <= p < 1: -2*y*(1-p)
p < -1: -4*y

Since the weight update here is w += eta * (-dloss/dp) * x, the middle region needs 2*eta*(y - p*y) (not eta*(y - p*y)), and the p >= 1 region needs no update at all. The p < -1 branch (4.0*eta*y) was already correct and is unchanged.

Testing

Added three new test cases to test/test.model.js covering each region (p < -1, -1 <= p < 1, p >= 1), confirming:

the factor-of-2 fix in the quadratic region,
the model weight vector is left unchanged when p >= 1,
the existing p < -1 branch is unaffected.

$ node test/test.model.js
TAP version 13
# main export is a function
ok 1 ...
ok 2 main export is a function
# the modified Huber loss update includes a `2` factor when `-1 <= y*f(x) < 1` (see stdlib-js/stdlib#13111)
ok 3 includes the `2` factor in the update
# the modified Huber loss does not update the model weight vector when `y*f(x) >= 1`
ok 4 leaves the model weight vector unchanged
# the modified Huber loss update matches the existing `-4*eta*y` branch when `y*f(x) < -1`
ok 5 applies the unchanged `d < -1` branch

1..5
# tests 5
# pass  5

eslint on both touched files is clean (aside from a pre-existing, unrelated spelling warning on "pegasos").

Related Issues

Fixes #13111

Checklist

Read, understood, and followed the contributing guidelines.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

Yes

If you answered "yes" above, how did you use AI assistance?

Code generation (e.g., when writing an implementation or fixing a bug)
Test/benchmark generation
Documentation (including examples)
Research and understanding

Disclosure

This PR was written primarily by Claude Code.

@stdlib-js/reviewers

…cation` The `else` branch of `_modifiedHuberLoss` was missing a factor of `2` and was applied unconditionally for `d = y*f(x) < 1`, including the `d >= 1` region where the loss (and its gradient) is exactly zero. Per the modified Huber loss definition (Wikipedia, "Variant for classification"; scikit-learn's `_sgd_fast.pyx.tp` `ModifiedHuber`), the gradient of the loss w.r.t. the decision value `p = y*f(x)` is: - `p >= 1`: `0` - `-1 <= p < 1`: `-2*y*(1-p)` - `p < -1`: `-4*y` The weight update is `w += eta * (-dloss/dp) * x`, so the middle region needs `2*eta*(y - p*y)`, not `eta*(y - p*y)`, and the `p >= 1` region needs no update at all. Fixes stdlib-js#13111

stdlib-bot · 2026-09-18T06:49:07Z

Hello! Thank you for your contribution to stdlib.

We noticed that the contributing guidelines acknowledgment is missing from your pull request. Here's what you need to do:

Please read our contributing guidelines.
Update your pull request description to include this checked box:

- [x] Read, understood, and followed the [contributing guidelines](https://github.com/stdlib-js/stdlib/blob/develop/CONTRIBUTING.md)

This acknowledgment confirms that you've read the guidelines, which include:

The developer's certificate of origin
Your agreement to license your contributions under the project's terms

We can't review or accept contributions without this acknowledgment.

Thank you for your understanding and cooperation. We look forward to reviewing your contribution!

Abhist17 · 2026-09-26T18:53:21Z

Gentle ping on #15317 — it's been 9 days. Is there anything you'd like changed before it can be reviewed? Happy to rebase or split it if that helps.

kgryte · 2026-09-27T08:53:54Z

@nakul-krishnakumar Would you mind doing an initial review of this PR? Cheers!

nakul-krishnakumar · 2026-09-28T20:34:43Z

 	if ( d < -1.0 ) {
 		this._add( x, 4.0*eta*y );
-	} else {
-		this._add( x, eta*( y-(d*y) ) );
+	} else if ( d < 1.0 ) {
+		this._add( x, 2.0*eta*( y-(d*y) ) );
 	}


This does not look correct to me as here else if comes after an else. According to sklearn, the correct branching should be:

if ( d >= 1.0 ) { // no update } else if ( d >= -1.0 ) { this._add( x, 2.0*eta*( y-(d*y) ) ); } else { this._add( x, 4.0*eta*y ); }

Reference

To make it simpler we can do this:

if ( d < -1.0 ) { this._add( x, 4.0*eta*y ); } else if ( d < 1.0 ) { this._add( x, 2.0*eta*( y-(d*y) ) ); }

Another reference is our @stdlib/ml/base/loss/float64/modified-huber-gradient implementation.

nakul-krishnakumar · 2026-09-28T20:41:38Z

+// FUNCTIONS //
+
+/**
+* Returns a model configured to use the modified Huber loss function with a constant learning rate and no regularization.
+*
+* @private
+* @param {number} eta - learning rate
+* @returns {Model} model instance
+*/
+function createModel( eta ) {
+	return new Model( 1, {
+		'lambda': 0.0,
+		'learningRate': [ 'constant', eta ],
+		'loss': 'modifiedHuber',
+		'intercept': false
+	});
+}
+
+


I believe this is not the convention we follow, instead we can create the model object in each test case itself. For better clarity refer this: ml/incr/kmeans

nakul-krishnakumar · 2026-09-28T20:46:09Z

 });

-// TODO: add tests
+tape( 'the modified Huber loss update includes a `2` factor when `-1 <= y*f(x) < 1` (see stdlib-js/stdlib#13111)', function test( t ) {


Instead of keeping three separate tests for just modified huber loss, I would suggest to write a single test, maybe something like:

tape( 'the function supports modified Huber loss', function test( t ) { // test case 1 // test case 2 // test case 3 }

And this test can have multiple test cases, each testing different branches of the loss function. I would also recommend to only add this test as part of this PR and then later open a new PR to add all the other tests for this package.

nakul-krishnakumar · 2026-09-28T20:51:12Z

Left some comments @Abhist17.

cc: @kgryte

Address review feedback: create each model inside the test case and cover the three branches of the loss in a single test.

Abhist17 · 2026-09-29T16:19:47Z

@nakul-krishnakumar thanks for the review.

Branching: the code on the branch already has the form you suggested. The else if in the diff is } else if ( d < 1.0 ) { replacing the old } else {, so the current code is if ( d < -1.0 ) { 4*eta*y } else if ( d < 1.0 ) { 2*eta*( y - d*y ) } with no update for d >= 1. That matches sklearn's ModifiedHuber.dloss and ml/base/loss/float64/modified-huber-gradient.
Tests: done in 1f23797. The createModel helper is gone, each case creates its own Model, and the three branches are now covered by a single the function supports modified Huber loss test. Other tests for the package can come in a separate PR, as you suggested.

stdlib-bot · 2026-09-29T16:21:42Z

Coverage Report

Package	Statements	Branches	Functions	Lines
ml/incr/binary-classification	$\\color{red}779/1023$ $\\color{green}+0.00\\%$	$\\color{red}41/46$ $\\color{green}+0.00\\%$	$\\color{red}9/22$ $\\color{green}+0.00\\%$	$\\color{red}779/1023$ $\\color{green}+0.00\\%$

The above coverage report was generated for the changes in this PR.

nakul-krishnakumar · 2026-09-29T20:16:39Z

+	};
+	x = array( new Float64Array( [ 1.0 ] ) );
+
+	// Case 1: `-1 <= y*f(x) < 1` (quadratically smoothed region), where the update is `2*eta*( y - d*y )` (see stdlib-js/stdlib#13111):


Suggested change

// Case 1: `-1 <= y*f(x) < 1` (quadratically smoothed region), where the update is `2*eta*( y - d*y )` (see stdlib-js/stdlib#13111):

// Case 1: `-1 <= y*f(x) < 1` (quadratically smoothed region):

Applied in 61f5267.

nakul-krishnakumar · 2026-09-29T20:17:11Z

+	model.update( x, 1 );
+	t.strictEqual( model._weights[ 0 ], 2.0, 'returns expected value' ); // eslint-disable-line no-underscore-dangle
+
+	// Case 3: `y*f(x) < -1`, where the update is `4*eta*y`:


Suggested change

// Case 3: `y*f(x) < -1`, where the update is `4*eta*y`:

// Case 3: `y*f(x) < -1`:

Applied in 61f5267.

nakul-krishnakumar · 2026-09-29T20:21:00Z

@nakul-krishnakumar thanks for the review.

* **Branching:** the code on the branch already has the form you suggested. The `else if` in the diff is `} else if ( d < 1.0 ) {` replacing the old `} else {`, so the current code is `if ( d < -1.0 ) { 4*eta*y } else if ( d < 1.0 ) { 2*eta*( y - d*y ) }` with no update for `d >= 1`. That matches sklearn's `ModifiedHuber.dloss` and `ml/base/loss/float64/modified-huber-gradient`.

* **Tests:** done in [1f23797](https://github.com/stdlib-js/stdlib/commit/1f237971a55e30b8bdc99cef628e76254e9248a8). The `createModel` helper is gone, each case creates its own `Model`, and the three branches are now covered by a single `the function supports modified Huber loss` test. Other tests for the package can come in a separate PR, as you suggested.

Oh right! My bad about the first point. 😅
Thank you for your contribution on this issue @Abhist17. I have left some more comments, other than that LGTM!

cc: @kgryte

Co-authored-by: Nakul Krishnakumar <nakul-krishnakumar@users.noreply.github.com>

Abhist17 requested a review from a team September 18, 2026 06:48

stdlib-bot added First-time Contributor A pull request from a contributor who has never previously committed to the project repository. Needs Review A pull request which needs code review. labels Sep 18, 2026

stdlib-bot removed the First-time Contributor A pull request from a contributor who has never previously committed to the project repository. label Sep 23, 2026

nakul-krishnakumar reviewed Sep 28, 2026

View reviewed changes

test: consolidate the modified Huber loss tests

1f23797

Address review feedback: create each model inside the test case and cover the three branches of the loss in a single test.

nakul-krishnakumar reviewed Sep 29, 2026

View reviewed changes

test: simplify case comments

61f5267

Co-authored-by: Nakul Krishnakumar <nakul-krishnakumar@users.noreply.github.com>

	// Case 1: `-1 <= yf(x) < 1` (quadratically smoothed region), where the update is `2eta( y - dy )` (see stdlib-js/stdlib#13111):
	// Case 1: `-1 <= y*f(x) < 1` (quadratically smoothed region):

	// Case 3: `yf(x) < -1`, where the update is `4eta*y`:
	// Case 3: `y*f(x) < -1`:

Conversation

Abhist17 commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Related Issues

Checklist

AI Assistance

Disclosure

Uh oh!

stdlib-bot commented Sep 18, 2026

Uh oh!

Abhist17 commented Sep 26, 2026

Uh oh!

kgryte commented Sep 27, 2026

Uh oh!

nakul-krishnakumar Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakul-krishnakumar Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

nakul-krishnakumar Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakul-krishnakumar Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakul-krishnakumar commented Sep 28, 2026

Uh oh!

Abhist17 commented Sep 29, 2026

Uh oh!

stdlib-bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Uh oh!

nakul-krishnakumar Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Abhist17 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

nakul-krishnakumar Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

Abhist17 Oct 13, 2026

Choose a reason for hiding this comment

Uh oh!

nakul-krishnakumar commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Abhist17 commented Sep 18, 2026 •

edited

Loading

nakul-krishnakumar Sep 28, 2026 •

edited

Loading

nakul-krishnakumar Sep 28, 2026 •

edited

Loading

nakul-krishnakumar Sep 28, 2026 •

edited

Loading

stdlib-bot commented Sep 29, 2026 •

edited

Loading

nakul-krishnakumar commented Sep 29, 2026 •

edited

Loading