Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 96.71% 96.81% +0.09%
==========================================
Files 33 36 +3
Lines 2038 2226 +188
==========================================
+ Hits 1971 2155 +184
- Misses 67 71 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
3 issues found across 9 files
Confidence score: 3/5
- Potential runtime failure in
src/tdhook/latent/dimension_estimation/local_pca.pyif tensors require gradients, since the neighborhood is converted to NumPy without detaching - Neighbor selection edge cases in
local_pca.py(self/too-close points markedinfbut still selected) could lead to fewer than k valid neighbors and unstable estimates - Score reflects a few medium-severity, user-impacting correctness risks in dimension estimation rather than merge-blocking defects
- Pay close attention to
src/tdhook/latent/dimension_estimation/local_pca.pyandsrc/tdhook/latent/dimension_estimation/ca_pca.py- gradient detachment and neighbor selection consistency
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/tdhook/latent/dimension_estimation/local_pca.py">
<violation number="1" location="src/tdhook/latent/dimension_estimation/local_pca.py:101">
P2: `sorted_neighbors` marks self/too-close points as `inf`, but `neighbor_idx = indices[i, :k]` never checks whether those indices correspond to finite distances. If fewer than k valid neighbors exist (e.g., duplicates within `eps`), this will include excluded points instead of returning NaN or skipping the neighborhood.</violation>
<violation number="2" location="src/tdhook/latent/dimension_estimation/local_pca.py:103">
P2: Detach the neighborhood tensor before converting to NumPy so this works with tensors that require gradients.</violation>
</file>
<file name="src/tdhook/latent/dimension_estimation/ca_pca.py">
<violation number="1" location="src/tdhook/latent/dimension_estimation/ca_pca.py:88">
P2: PCA neighborhood uses only k neighbors even though CA-PCA is described as using k+1 neighbors and r is computed from k and k+1 distances. This inconsistency can bias the dimension estimate; include the (k+1)-th neighbor in the PCA neighborhood.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/source/notebooks/methods/dimension-estimation.ipynb">
<violation number="1" location="docs/source/notebooks/methods/dimension-estimation.ipynb:551">
P2: Indexing a torch tensor with a NumPy boolean mask (`labels == d`) is not reliably supported and can throw TypeError or mis-index. Convert the mask to a torch boolean tensor before indexing to avoid runtime errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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
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.
What does this PR do?
Key insights about the PR.
Linked Issues
Checklist
Summary by cubic
Adds LocalPCA and curvature-adjusted CA‑PCA estimators for per‑point intrinsic dimension from k‑NN neighborhoods. Ships a new notebook with synthetic data, MNIST, TwoNN plots, clearer per‑point visuals, and a per‑label MNIST analysis.
New Features
Dependencies
Written for commit 1594373. Summary will update on new commits.