Fix 1/N floor in effective-rank collapse score after mean-centering#8997
Open
rubenG1009 wants to merge 1 commit into
Open
Fix 1/N floor in effective-rank collapse score after mean-centering#8997rubenG1009 wants to merge 1 commit into
rubenG1009 wants to merge 1 commit into
Conversation
…ering _effective_rank_score mean-centers the embedding matrix, which caps the attainable rank at min(N-1, D), but normalized by min(N, D). That imposed a hard 1/N floor on the score whenever N <= D: isotropic embeddings at N=4, D=768 scored 0.2503 and two maximally-spread points scored exactly 0.50, where the correct answer is ~0. _per_class_rank is affected more, since per-class N is smaller and the floor is therefore larger. The fix is a no-op for N > D, where both expressions equal D. Also updates test_formula_matches_manual, which mirrored the old normalization inline. Its fixture is N=20 > D=16, so it passes either way, but it would otherwise encode the old formula. Signed-off-by: rubenuni1009 <183279777+rubenG1009@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThe effective-rank collapse score now normalizes by Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/metrics/test_embedding_collapse.py`:
- Around line 231-251: Replace the inline comments in
test_effective_rank_no_collapse_when_n_leq_d,
test_effective_rank_two_distinct_samples_report_no_collapse, and
test_effective_rank_unchanged_when_n_gt_d with Google-style method docstrings
describing the test inputs, expected score behavior, return value, and any
raised exceptions as applicable. Preserve the existing test logic and
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de1aab56-425f-4a98-9a56-cfe1a5dd5bd2
📒 Files selected for processing (2)
monai/metrics/embedding_collapse.pytests/metrics/test_embedding_collapse.py
Comment on lines
+231
to
+251
| def test_effective_rank_no_collapse_when_n_leq_d(self): | ||
| # Isotropic embeddings have NO dimensional collapse -> score must be ~0. | ||
| # RED on dev: returns ~0.25 (the 1/N floor). GREEN after fix: ~0.0004. | ||
| torch.manual_seed(0) | ||
| score = _effective_rank_score(torch.randn(4, 768)) | ||
| self.assertLess(float(score), 0.05) | ||
|
|
||
| def test_effective_rank_two_distinct_samples_report_no_collapse(self): | ||
| # Two maximally-spread points span the only subspace 2 samples CAN span. | ||
| # RED on dev: returns exactly 0.5 ("50% collapsed"). GREEN after fix: 0.0. | ||
| emb = torch.zeros(2, 768) | ||
| emb[0, 0] = 1.0 | ||
| emb[1, 0] = -1.0 | ||
| self.assertAlmostEqual(float(_effective_rank_score(emb)), 0.0, places=5) | ||
|
|
||
| def test_effective_rank_unchanged_when_n_gt_d(self): | ||
| # Regression guard: the fix MUST be a no-op where the code is already correct. | ||
| torch.manual_seed(0) | ||
| score = _effective_rank_score(torch.randn(1024, 768)) | ||
| self.assertAlmostEqual(float(score), 0.1234, places=3) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add missing docstrings to test definitions.
Convert the inline comments to Google-style docstrings for the new test methods. As per path instructions, Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings.
📝 Proposed fix
- def test_effective_rank_no_collapse_when_n_leq_d(self):
- # Isotropic embeddings have NO dimensional collapse -> score must be ~0.
- # RED on dev: returns ~0.25 (the 1/N floor). GREEN after fix: ~0.0004.
+ def test_effective_rank_no_collapse_when_n_leq_d(self):
+ """Isotropic embeddings have no dimensional collapse (score must be ~0).
+
+ Returns:
+ None
+ """
torch.manual_seed(0)
score = _effective_rank_score(torch.randn(4, 768))
self.assertLess(float(score), 0.05)
- def test_effective_rank_two_distinct_samples_report_no_collapse(self):
- # Two maximally-spread points span the only subspace 2 samples CAN span.
- # RED on dev: returns exactly 0.5 ("50% collapsed"). GREEN after fix: 0.0.
+ def test_effective_rank_two_distinct_samples_report_no_collapse(self):
+ """Two maximally-spread points span the only subspace 2 samples can span.
+
+ Returns:
+ None
+ """
emb = torch.zeros(2, 768)
emb[0, 0] = 1.0
emb[1, 0] = -1.0
self.assertAlmostEqual(float(_effective_rank_score(emb)), 0.0, places=5)
- def test_effective_rank_unchanged_when_n_gt_d(self):
- # Regression guard: the fix MUST be a no-op where the code is already correct.
+ def test_effective_rank_unchanged_when_n_gt_d(self):
+ """Regression guard: the fix must be a no-op when n > d.
+
+ Returns:
+ None
+ """
torch.manual_seed(0)
score = _effective_rank_score(torch.randn(1024, 768))
self.assertAlmostEqual(float(score), 0.1234, places=3)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_effective_rank_no_collapse_when_n_leq_d(self): | |
| # Isotropic embeddings have NO dimensional collapse -> score must be ~0. | |
| # RED on dev: returns ~0.25 (the 1/N floor). GREEN after fix: ~0.0004. | |
| torch.manual_seed(0) | |
| score = _effective_rank_score(torch.randn(4, 768)) | |
| self.assertLess(float(score), 0.05) | |
| def test_effective_rank_two_distinct_samples_report_no_collapse(self): | |
| # Two maximally-spread points span the only subspace 2 samples CAN span. | |
| # RED on dev: returns exactly 0.5 ("50% collapsed"). GREEN after fix: 0.0. | |
| emb = torch.zeros(2, 768) | |
| emb[0, 0] = 1.0 | |
| emb[1, 0] = -1.0 | |
| self.assertAlmostEqual(float(_effective_rank_score(emb)), 0.0, places=5) | |
| def test_effective_rank_unchanged_when_n_gt_d(self): | |
| # Regression guard: the fix MUST be a no-op where the code is already correct. | |
| torch.manual_seed(0) | |
| score = _effective_rank_score(torch.randn(1024, 768)) | |
| self.assertAlmostEqual(float(score), 0.1234, places=3) | |
| def test_effective_rank_no_collapse_when_n_leq_d(self): | |
| """Isotropic embeddings have no dimensional collapse (score must be ~0). | |
| Returns: | |
| None | |
| """ | |
| torch.manual_seed(0) | |
| score = _effective_rank_score(torch.randn(4, 768)) | |
| self.assertLess(float(score), 0.05) | |
| def test_effective_rank_two_distinct_samples_report_no_collapse(self): | |
| """Two maximally-spread points span the only subspace 2 samples can span. | |
| Returns: | |
| None | |
| """ | |
| emb = torch.zeros(2, 768) | |
| emb[0, 0] = 1.0 | |
| emb[1, 0] = -1.0 | |
| self.assertAlmostEqual(float(_effective_rank_score(emb)), 0.0, places=5) | |
| def test_effective_rank_unchanged_when_n_gt_d(self): | |
| """Regression guard: the fix must be a no-op when n > d. | |
| Returns: | |
| None | |
| """ | |
| torch.manual_seed(0) | |
| score = _effective_rank_score(torch.randn(1024, 768)) | |
| self.assertAlmostEqual(float(score), 0.1234, places=3) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/metrics/test_embedding_collapse.py` around lines 231 - 251, Replace the
inline comments in test_effective_rank_no_collapse_when_n_leq_d,
test_effective_rank_two_distinct_samples_report_no_collapse, and
test_effective_rank_unchanged_when_n_gt_d with Google-style method docstrings
describing the test inputs, expected score behavior, return value, and any
raised exceptions as applicable. Preserve the existing test logic and
assertions.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8996
Description
_effective_rank_scoremean-centers the embedding matrix (L290), which caps its attainable rank atmin(N-1, D), but normalized the effective rank bymin(N, D)(L298).Since the score is
1 - eff_rank/max_rank, that imposed a hard floor of1/NwheneverN <= D: isotropic embeddings at N=4, D=768 scored 0.2503, and two maximally-spread points scored exactly 0.50 ("50% collapsed"), where the correct answer is ~0. The metric could not report "no collapse" in precisely the regime it was designed for._per_class_rankis affected more, since per-class N is smaller and the floor is therefore larger.The observed floor matches
1 - (N-1)/N = 1/Nexactly.Why
min(N-1, D)Mean-centering forces the rows to sum to zero, which is a linear dependency: if you know N-1 rows, the last one is determined. So
rank(centered) <= min(N-1, D). Normalizing by the attainable maximum removes the artifact.The fix is a no-op for
N > D, wheremin(N-1, D)andmin(N, D)both equalD. Verified: a 1024x768 matrix scores 0.1234 before and after.Tests
Three tests added to
TestEffectiveRankScore, all RED ondevand GREEN after the fix (except the third, which is a regression guard and passes both ways):test_effective_rank_no_collapse_when_n_leq_d- 4x768 isotropic: 0.2503 -> ~0.0004test_effective_rank_two_distinct_samples_report_no_collapse- 2x768: 0.5 -> 0.0test_effective_rank_unchanged_when_n_gt_d- 1024x768 unchanged at 0.1234 (proves no regression)Full suite: 48 passed, 2 skipped (sklearn not installed locally).
Note on the modified test
test_formula_matches_manualmirrored the old normalization inline (min(20, 16)). Its fixture is N=20 > D=16, so it passes either way, but it would otherwise encode the old formula. Updated tomin(20 - 1, 16)- flagging it explicitly rather than changing an assertion silently.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.