Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug in remove #63

Merged
merged 1 commit into from
May 28, 2018
Merged

Fixed bug in remove #63

merged 1 commit into from
May 28, 2018

Conversation

mhsaul
Copy link
Contributor

@mhsaul mhsaul commented Nov 30, 2017

The bug occurs when replacing a removed node. If child consumes all the nodes in s_center, the node is completely removed from the tree which allows its parent to become in balanced. Re-balancing will happen in lines 399-400 and cause the parent to be rotated down to be the right child of its current left child. The problem here is that if the parent contains any overlapping intervals with child.x_center, these intervals will not be added to child, since the check for overlaps in lines 403-405 will look at the new parent (i.e. the left child of the old parent). Thus, the invariant that no node overlaps the x_center of any of its ancestors is broken.

@escalonn
Copy link
Contributor

escalonn commented Dec 2, 2017

some overlap with #42

@mhsaul
Copy link
Contributor Author

mhsaul commented Dec 3, 2017

@escalonn the first bug fix was a copy of the code changes from your PR #42. I think the second fix will resolve the rest of the issues with the remove function. I tested it with the file issue41a_test.txt from Issue #41 and it fixed the problem.

@chaimleib chaimleib changed the base branch from master to dev December 10, 2017 20:24
@FalkoHof
Copy link

FalkoHof commented May 24, 2018

Hey, I encountered the same problem and this fix worked for me perfectly. However it has not yet been merged into the master branch or the current releases?

@chaimleib chaimleib merged commit 30e7bed into chaimleib:dev May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants