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

Fixes point_search when unique is false #14

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

danielpetri1
Copy link

This change implements point_search iteratively to ensure a stack overflow can not occur.

It also makes the method use the passed unique parameter, as the recursive call was previously defaulting unique to true:

itv = [(5...20), (15.6...20), (15.7...20), (15.7...20)]
t = IntervalTree::Tree.new(itv)
p t.search(15.7)     #=> [5...20, 15.6...20, 15.7...20]
p t.search(15.7, unique: false) #=> Should be [5...20, 15.6...20, 15.7...20, 15.7...20], not [5...20, 15.6...20, 15.7...20] again

Additionally, result.uniq is invoked at the end of the method for faster query speeds.

This change implements point_search iteratively to ensure a stack overflow can not occur.

It also makes the method use the passed unique parameter, as the recursive call was previously defaulting unique to true:

itv = [(5...20), (15.6...20), (15.7...20), (15.7...20)]
t = IntervalTree::Tree.new(itv)
p t.search(15.7)     #=> [5...20, 15.6...20, 15.7...20]
p t.search(15.7, unique: false) #=> Should be [5...20, 15.6...20, 15.7...20, 15.7...20], not [5...20, 15.6...20, 15.7...20] again

Additionally, result.uniq is invoked at the end of the method for faster query speeds.
Fixed point_search Indentation
Co-authored-by: amarzot-yesware <[email protected]>
Copy link
Contributor

@ZimbiX ZimbiX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers; good thinking.

Given this includes the removal of #include?, from #13, let's rebase this once that PR is merged. In the meantime, would you mind adding a spec please? =)

Copy link
Contributor

@ZimbiX ZimbiX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I should have selected this, given the missing spec)

@danielpetri1
Copy link
Author

Hi @ZimbiX,
Thanks for reviewing the code. I added 3 example to showcase where unique was failing in my view.

In addition to that I also changed the text of the case in which uniq defaults to true and added one more example for it. Before, it said something along the lines of "given intervals with duplicates, return the duplicates in the result" - I am assuming duplicates should be removed from the intervals in this PR.

Cheers!

@danielpetri1 danielpetri1 deleted the patch-1 branch October 9, 2021 12:30
@danielpetri1 danielpetri1 restored the patch-1 branch October 9, 2021 13:49
@danielpetri1 danielpetri1 reopened this Oct 9, 2021
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.

3 participants