Fix a bug in removed index extraction in UniformSampling
#3323
Conversation
This file contains 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
Points that were removed due to large distance to existing leaf were not recorded in the removed_indices_ vector.
SergioRAgostinho
approved these changes
Sep 16, 2019
There was a problem hiding this comment.
LGTM. I still need to investigate the connection between this PR and #2022
It's the same. Just found this. #2022 (comment)
I went digging for the issue myself and found the bug. The issue was that we were not adding the points in the condition that their distance was bigger than the one for the index already stored on the leaf. The snippet below should help to clarify my lousy explanation. Your tests (thank you for writing them btw), are running green.
|
Oh, I completely forgot about that PR. Should have merged it two years ago! But instead kept the bug open, bumped into it, and wasted time re-implementing the same fix. This is really embarrassing 😞 |
|
Great. Sorry for not following up two years ago 😅 |
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.
This PR fixes a bug in removed index extraction in
UniformSampling. Points that were removed due to large distance to existing leaf were not recorded in theremoved_indices_vector. This was overlooked in #1902.There were no unit tests at all for
UniformSampling, so I added a new one that covers the functionality that this PR fixes.Closes #2022