Fix #29715: Removed >= in line 225 of manifold/_locally_linear #29716
Conversation
| @@ -222,7 +222,7 @@ def _locally_linear_embedding( | |||
| raise ValueError( | |||
| "output dimension must be less than or equal to input dimension" | |||
| ) | |||
| if n_neighbors >= N: | |||
| if n_neighbors > N: | |||
There was a problem hiding this comment.
I actually think since we're passing n_neighbors + 1 to NearestNeighbors here a few lines above,
the condition is correct, and the error message is wrong, i.e. here n_neighbors does NOT include the data point itself. (I find it quite confusing though)
cc @adam2392 maybe
There was a problem hiding this comment.
If the condition is incorrect, it seems like an in-line comment should be added, as I agree it's confusing.
Unfortunately, we can't just do n_neighbors += 1, as it's used below.
Why are we passing in n_neighbors + 1 into NearestNeighbors anyways? Def something worth clarifying in the code + then fix the error message.
There was a problem hiding this comment.
I believe that since n_neighbors is increased by 1 only in the NearestNeighbors function, it does not affect the original n_neighbors variable itself. Therefore, when the validation checks the value of n_neighbors, it will only check the original variable, not the incremented one used internally by NearestNeighbors.
The reason for incrementing n_neighbors by 1 in NearestNeighbors is to ensure that the point itself is included in the list of neighbors. This is because most nearest neighbors algorithms will include the point being queried as the first neighbor. By requesting n_neighbors + 1 neighbors, the algorithm guarantees that the actual n_neighbors closest points, excluding the query point itself, are returned.
There was a problem hiding this comment.
that tells me the condition is correct, but the message is wrong.
|
@AdrianoCLeao would you be able to apply the changes here? You need to fix the message and revert the condition to what it was. |
|
@AdrianoCLeao would you be able to fix this PR? As described above, you need to fix the message and revert the condition to what it was. |
Description
This pull request addresses the issue #29715 by fixing the condition in the
LocallyLinearEmbeddingclass that checks the relationship betweenn_neighborsandn_samples.Problem
The original code raised a
ValueErrorwhenn_neighborswas greater than or equal ton_samples, which caused confusion and incorrect error messages. Specifically, users encountered an error even whenn_neighborswas equal ton_samples, which should be a valid scenario based on the error message:Solution
The condition was updated to only raise a ValueError when n_neighbors is greater than n_samples, allowing n_neighbors to be equal to n_samples without raising an error:
This change aligns the behavior of the code with the expected usage, preventing unnecessary errors when n_neighbors == n_samples.
Tests
No documentation changes were necessary, as this is a minor fix to existing functionality.