Skip to content

Fix L1 distance to use Manhattan sum - #1998

Open
SahilKumar75 wants to merge 1 commit into
sbi-dev:mainfrom
SahilKumar75:fix/l1-manhattan-distance
Open

Fix L1 distance to use Manhattan sum#1998
SahilKumar75 wants to merge 1 commit into
sbi-dev:mainfrom
SahilKumar75:fix/l1-manhattan-distance

Conversation

@SahilKumar75

Copy link
Copy Markdown

Summary

  • compute L1 distance with a sum of absolute differences, matching the documented Manhattan-distance definition and l2's sum-based convention
  • add a regression test covering dimensionality scaling and agreement on a simple case

Testing

  • ruff check sbi/utils/metrics.py tests/metrics_test.py
  • ruff format --check sbi/utils/metrics.py tests/metrics_test.py
  • direct l1/l2 Manhattan-distance regression passed
  • full metrics test module could not be collected in this environment because pytest_harvest and nflows are unavailable

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9976e788-7f82-4c0f-8d80-b8b0200d6588

📥 Commits

Reviewing files that changed from the base of the PR and between 900821d and 69df3b4.

📒 Files selected for processing (2)
  • sbi/utils/metrics.py
  • tests/metrics_test.py

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The l1 metric now computes non-averaged Manhattan distance by summing absolute differences along the selected axis. Regression tests verify behavior when identical dimensions are added.

Changes

Metric distance behavior

Layer / File(s) Summary
Update L1 distance and regression coverage
sbi/utils/metrics.py, tests/metrics_test.py
l1 now sums absolute differences instead of averaging them. Tests verify the expected distance for one differing dimension and additional identical dimensions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 69df3

This change corrects L1 distance to use the documented Manhattan sum and adds regression coverage. No actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change to use the Manhattan sum for L1 distance.
Description check ✅ Passed The description explains the behavior change, regression test, and validation results, but it omits several template sections and checklist items.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SahilKumar75

Copy link
Copy Markdown
Author

The focused regression change is ready for maintainer review. The test suite coverage and formatting checks are complete.

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.

1 participant