Optimisation: Use heapify in MutableBinaryHeap #712
Conversation
rushabh-v
changed the title
|
@AquaIndigo can you review? |
|
CI fails so i suspect this is not correct |
|
Turned out I haven't updated Or does the issue intend to use |
I think that |
| vs = [4, 1, 3, 2, 16, 9, 10, 14, 8, 7] | ||
| vs2 = collect(enumerate(vs)) |
There was a problem hiding this comment.
The new implementation was overwriting vs. So, the tests were failing because of updated vs (The value of vs was not as expected because it was updated during some other test). So, I updated it to use separate vs and vs2 for each test (the values are still the same).
There was a problem hiding this comment.
It is not OK for MutableBinaryMinHeap(vs) to mutate vs.
We should add a copy to the constructor if that is the case.
(we can consider later adding a MutableBinaryMinHeap! or something that does do so. but lets not worry about it in this PR)
There was a problem hiding this comment.
list_values related tests (i.e., this one) started failing after adding copy in the constructor. the reason for that is because nodemap is not updated as I mentioned here. Those tests were not failing before because vs was being mutated.
I am trying to think if there is an efficient way to update nodemap after the heapify! call.
It might be that not being able to update nodemap efficiently (with O(n) implementation) is the reason they implemented it in O(nlogn) in the first place.
| h = MutableBinaryMaxHeap(vs) | ||
|
|
||
| @test length(h) == 10 | ||
| @test !isempty(h) | ||
| @test first(h) == 16 | ||
| @test isequal(list_values(h), vs) | ||
| @test isequal(heap_values(h), [16, 14, 10, 8, 7, 3, 9, 1, 4, 2]) | ||
| @test isequal(heap_values(h), [16, 14, 10, 8, 7, 9, 3, 2, 4, 1]) |
There was a problem hiding this comment.
I think this is just due to the difference in the implementation of heapify and the previous method _heap_bubble_up. Otherwise, there is no problem IMO, so I updated the test. (Let me know if that's not the case)
There was a problem hiding this comment.
Do these both follow the heap rule about being larger than children?
(I don't know how to read heaps when written in array form TBH)
There was a problem hiding this comment.
Yes, they both are valid heaps. I also confirmed it using this function here.
|
I ran the tests locally, it was all green. |
| @@ -121,6 +126,7 @@ end | |||
| end | |||
|
|
|||
| @testset "hmin / push! / pop!" begin | |||
| vs = [4, 1, 3, 2, 16, 9, 10, 14, 8, 7] | |||
There was a problem hiding this comment.
this is a good change anyway. Testsets hould be indipendent.
Though maybe we move this down to after ss is declared?
|
Sorry for taking so long to review this. It may also need rebasing to get out new CI setup to work |
Fixes: #639
Note: I was a little confused by the issue-description, so not sure if the changes I made are at the correct place.