Skip to content

Fix Huber Loss gradient scaling - #420

Merged
andrewdalpino merged 6 commits into
masterfrom
fix-huber-loss-norm
Aug 18, 2026
Merged

Fix Huber Loss gradient scaling#420
andrewdalpino merged 6 commits into
masterfrom
fix-huber-loss-norm

Conversation

@andrewdalpino

Copy link
Copy Markdown
Member

src/NeuralNet/CostFunctions/HuberLoss.php:110 — differentiate returns e/√(e²+α²) while compute implements α²(√(1+(e/α)²)−1) whose true gradient is α·e/√(α²+e²) (missing factor α). Constant scale error; direction correct.

@andrewdalpino
andrewdalpino requested review from a team and a lite review from Copilot August 15, 2026 22:03
@andrewdalpino andrewdalpino added the bug Something isn't working label Aug 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes the magnitude scaling bug in HuberLoss::differentiate() so the analytical gradient matches the implemented pseudo-Huber loss, and adds coverage to prevent regressions.

Changes:

  • Correct gradient scaling in HuberLoss::differentiate() by applying the missing alpha factor.
  • Add a numeric-gradient comparison test for HuberLoss with a non-1.0 alpha.
  • Update the changelog with a new 2.5.3 entry (currently includes an unrelated item).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/NeuralNet/CostFunctions/HuberLoss.php Applies the missing alpha factor so differentiate() matches the loss function’s true gradient magnitude.
tests/NeuralNet/CostFunctions/HuberLossTest.php Adds a numeric-gradient regression test for differentiate() under alpha=0.5.
CHANGELOG.md Adds a 2.5.3 entry, but currently lists an unrelated KNN fix not included in this PR.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/NeuralNet/CostFunctions/HuberLossTest.php
Comment thread CHANGELOG.md Outdated
@apphp

apphp commented Aug 16, 2026

Copy link
Copy Markdown

LGTM

@andrewdalpino

Copy link
Copy Markdown
Member Author

Should probably change the variable name of the local $alpha variable so it doesn't clash with the alpha hyper-parameter.

@andrewdalpino
andrewdalpino merged commit 68d3528 into master Aug 18, 2026
12 checks passed
@andrewdalpino
andrewdalpino deleted the fix-huber-loss-norm branch August 18, 2026 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants