Fix handling of many graph nodes at the same location #111
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
|
This should Fix #110 |
adolsalamanca
approved these changes
Jul 23, 2024
adolsalamanca
previously approved these changes
Jul 23, 2024
thedodd
reviewed
Jul 23, 2024
thedodd
reviewed
Jul 23, 2024
thedodd
previously approved these changes
Jul 23, 2024
When nodes have the same location, their distance to each other is 0. And, their distance to all their neighbors is the same. In the case with few maximum neighbors this can cause problems because the nodes end up linking to each other and getting cut off from the rest of the graph. We fix this by introducing a secondary "tie break" distance between nodes at the same location (based on on-disk distance). That's used to create a secondary ordering for equivalent nodes and allows the prune function to then properly cut away equivalent nodes so that your neighbor list doesn't get full of them. Note with SBQ, equivalence classes are actually not /so/ rare especially with low dimension counts.
alejandrodnm
approved these changes
Jul 24, 2024
| if distance_between_candidate_and_existing_neighbor < 0.0 + f32::EPSILON { | ||
| if distance_between_candidate_and_point < 0.0 + f32::EPSILON { | ||
| 1.0 | ||
| let factor = if distance_between_candidate_and_existing_neighbor |
There was a problem hiding this comment.
NIT: the prune_neighborgs functions is fairly big. Maybe you can abstract the factor calculation away to its own function. That way you can add docs specific to the factor calculation. Also, you can use guard clauses to remove some of the ifs.
There was a problem hiding this comment.
I'm going to ignore this for now. I think we may refactor this later anyway.
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.
When nodes have the same location, their distance to each other is 0. And, their distance to all their neighbors is the same. In the case with few maximum neighbors this can cause problems because the nodes end up linking to each other and getting cut off from the rest of the graph.
We fix this by introducing a secondary "tie break" distance between nodes at the same location (based on on-disk distance). That's used to create a secondary ordering for equivalent nodes and allows the prune function to then properly cut away equivalent nodes so that your neighbor list doesn't get full of them.
Note with SBQ, equivalence classes are actually not /so/ rare especially with low dimension counts.