Skip to content

Correct p-norm computation in triplet_loss#3613

Merged
zcbenz merged 1 commit into
ml-explore:mainfrom
pchintar:triplet-loss-p-norm
Jun 4, 2026
Merged

Correct p-norm computation in triplet_loss#3613
zcbenz merged 1 commit into
ml-explore:mainfrom
pchintar:triplet-loss-p-norm

Conversation

@pchintar

@pchintar pchintar commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Description

nn.losses.triplet_loss exposes a configurable norm degree through the p argument and documents the distance term as a p-norm:

||A - P||_p

However, the current distance computation is:

mx.sqrt(mx.power(anchors - positives, p).sum(axis) + eps)

which only matches the documented p-norm when p=2. For other values of p, it can produce incorrect results and even NaN values.

This change updates the distance computation to use the standard p-norm:

(sum(abs(x - y) ** p) + eps) ** (1 / p)

which matches both the function documentation and MLX's linalg.norm behavior.

On a sidenote, I also noticed that the LayerNorm, GroupNorm, and BatchNorm docstrings in normalization.py had epsilon outside the square root (\sqrt{Var[x]} + \epsilon) which doesn't match the standard normalization formula, so I changed it to (\sqrt{Var[x] + \epsilon}) instead.

Changes

  • Corrected the p-norm computation in nn.losses.triplet_loss
  • Added test coverage for non-default p values

Testing

python -m unittest python.tests.test_losses.TestLosses.test_triplet_loss

cc @zcbenz

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me, thanks for fixing this!

@zcbenz zcbenz force-pushed the triplet-loss-p-norm branch from 42ec0e7 to b69b9dd Compare June 4, 2026 05:09
@zcbenz zcbenz merged commit 6ea7a00 into ml-explore:main Jun 4, 2026
16 checks passed
@pchintar

pchintar commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

@zcbenz thnx again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants