DataStructures.jl icon indicating copy to clipboard operation
DataStructures.jl copied to clipboard

Binary heap constructor

Open hiemstar opened this issue 5 years ago • 4 comments

Modified internal constructor of BinaryHeap:

function BinaryHeap{T,Comp}(::Comp, xs::AbstractVector{T}) where {T,Comp}
    valtree = _make_binary_heap(Comp(), T, xs)
    new{T,Comp}(Comp(), valtree)
end

to

function BinaryHeap{T,Comp}(comp::Comp, xs::AbstractVector{T}) where {T,Comp}
    valtree = _make_binary_heap(comp, T, xs)
    new{T,Comp}(comp, valtree)
end

This makes it possible to heapify an array based on data not stored in the array itself.

hiemstar avatar Dec 24 '19 21:12 hiemstar

Codecov Report

Merging #564 into master will decrease coverage by 0.04%. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
- Coverage   87.82%   87.78%   -0.05%     
==========================================
  Files          32       32              
  Lines        2087     2088       +1     
==========================================
  Hits         1833     1833              
- Misses        254      255       +1
Impacted Files Coverage Δ
src/heaps/binary_heap.jl 97.72% <66.66%> (-2.28%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 685d33d...0a99afd. Read the comment docs.

codecov[bot] avatar Dec 24 '19 21:12 codecov[bot]

@dillondaudert can you review?

oxinabox avatar Feb 17 '20 10:02 oxinabox

Hi, thanks for the PR. Could you clarify with examples what exactly this change allows?

You'll also want to make sure that all the heaps (including the MinMax heap) follow the same API and have unit tests to cover any changed functionality.

dillondaudert avatar Feb 20 '20 16:02 dillondaudert

@hiemstar It appears to me that what this PR does is allows you to instantiate a BinaryMinHeap that has the max heap property and vice versa. To me, this would break the expected behaviour of the heap. Can you clarify what you intend this PR to do?

jakobnissen avatar Mar 31 '20 09:03 jakobnissen