Fix bugs in Scholar.Manifold.Trimap - #367
Merged
Merged
Conversation
in1d/2 sorts tensor1 so it can binary search each element, then applies the sorting permutation to the answers a second time instead of inverting it. The mask comes back permuted: against plain set membership it disagrees on 167 of 300 random inputs. Its only caller, rejection_sample/4, reads the mask through Nx.any/1 for the loop condition, and a permutation does not change that, so sampled outliers still never land in the reject set. What does change is which slots a retry round overwrites, so the drawn indices differ whenever the first draw collides.
find_scaled_neighbors/3 takes the local scale from the 4th to 6th nearest neighbor with a fixed distances[[.., 3..5]] slice, which assumes the neighbor matrix always has at least six columns. It has min(num_inliers + 50, n) + 1 of them, so a dataset of three or four points raises "index 5 is out of bounds" even though transform/2 documents and guards for more than two points. Clamp the slice to the columns that exist, so the scale falls back to however many of those neighbors the dataset has.
Inputs with more than @dim_pca features are supposed to be reduced before the triplets are sampled. That branch had never run, and it was wrong in four ways. It called the private Nx.LinAlg.SVD.svd/2 with full_matrices: false. That module takes full_matrices? and has no defaults, so the option was read as nil and building the graph raised a CompileError. Every input wider than 100 features failed. The reduction itself multiplied u, s and vt back together, which reconstructs the input in its original feature space instead of projecting it onto the principal components. The reference implementation projects, and the embedding is initialized from the leading columns of the result, so the initialization was reading original features rather than components. The slice was also inclusive, keeping 101 components rather than 100, and the reduced tensor was rebound inside the case clause so it never reached the initialization at all. Use the public Nx.LinAlg.svd/2, keep u * s, clamp the component count to the rank the data can support, and return the reduced tensor from the case. Checked against numpy: the projection matches an exact SVD to 2.9e-13 in f64.
generate_triplets/3 branches on num_random > 0 so that a run can use only the nearest neighbor triplets, but the option is typed :pos_integer, so passing 0 is rejected during validation and that branch can never be taken.
rejection_sample/4 draws num_inliers * num_outliers outliers per anchor, rejecting the anchor and its own inliers and discarding any draw that repeats a value already held. Once every acceptable index is already in hand, no further draw is ever accepted, and the loop has no iteration bound, so transform/2 never returns and reports nothing. That state is reachable whenever the request is wider than the pool of points outside an anchor's neighborhood. With the defaults it needs 50 outliers, so any dataset with fewer than 61 points can hang. Reject the impossible request up front, unless triplets were supplied and no sampling happens.
A supplied init_embeddings is used as given, so its width silently decides the size of the embedded space and num_components is ignored. Asking for two components while passing three columns returns a three column embedding.
Contributor
Author
|
One thing I deliberately left out of this PR is an issue around the interaction While testing the different combinations, I found that some combinations exposed
I have not included a fix for this in the PR because I still need to investigate |
The limit only surfaced when the new guard raised.
Contributor
|
💚 💙 💜 💛 ❤️ |
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
2 participants
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.
in1d/2returns its mask permuted. It applies the sort permutation againinstead of inverting it. Wrong on 167 of 300 random inputs.
distances[[.., 3..5]]slice, assuming at least six neighbor columns.full_matrices:to a private SVD that takesfull_matrices?:. Behind thecrash it also reconstructed instead of projecting, kept 101 components instead
of 100, and rebound the result inside the
caseso it never reached theinitialization.
num_random: 0is rejected.generate_triplets/3branches onnum_random > 0, but the option is typed:pos_integer.rejection_sample/4neverrepeats a draw and has no iteration bound. The defaults want 50 outliers, so
any dataset under 61 points can hang with no output.
init_embeddingssilently overridesnum_components. Three columns whileasking for two returned a three column embedding.
One behaviour change worth calling out: some configurations that returned a
result on main now raise.
rejection_sample/4fillsnum_inliers * num_outliersslots from the pool of points outside an anchor's neighbourhood,never repeating a value, so when the request is wider than that pool it is
unsatisfiable by pigeonhole. Those runs only completed by emitting duplicate
outliers, and with
in1d/2fixed they hang instead, which is why the guardrejects them up front.
The PCA projection matches an exact numpy SVD to 2.9e-13 in f64. Every fix has a
test that fails when only that fix is reverted. Full suite: 343 doctests, 662
tests, 0 failures