JuliaLang / Compat.jl

Compatibility across Julia versions
Other
144 stars 117 forks source link

Compat for keepat! #750

Closed bkamins closed 2 years ago

bkamins commented 3 years ago

Is keepat! going to be supported by Compat.jl? x-ref https://github.com/JuliaLang/julia/pull/36229

bkamins commented 2 years ago

This is the code that should be added:

keepat!(B::BitVector, inds) = _keepat!(B, inds)
keepat!(B::BitVector, inds::AbstractVector{Bool}) = _keepat!(B, inds)
keepat!(a::Vector, inds) = _keepat!(a, inds)
keepat!(a::Vector, m::AbstractVector{Bool}) = _keepat!(a, m)

function _keepat!(a::AbstractVector, inds)
    local prev
    i = firstindex(a)
    for k in inds
        if @isdefined(prev)
            prev < k || throw(ArgumentError("indices must be unique and sorted"))
        end
        ak = a[k] # must happen even when i==k for bounds checking
        if i != k
            @inbounds a[i] = ak # k > i, so a[i] is inbounds
        end
        prev = k
        i = nextind(a, i)
    end
    deleteat!(a, i:lastindex(a))
    return a
end

function _keepat!(a::AbstractVector, m::AbstractVector{Bool})
    length(m) == length(a) || throw(BoundsError(a, m))
    j = firstindex(a)
    for i in eachindex(a, m)
        @inbounds begin
            if m[i]
                i == j || (a[j] = a[i])
                j = nextind(a, j)
            end
        end
    end
    deleteat!(a, j:lastindex(a))
end

The issue is that it was added in two shots: https://github.com/JuliaLang/julia/pull/36229 and https://github.com/JuliaLang/julia/pull/42351 so the question is how the "bounds" for including this code in Compat.jl should be set.

@nalimilan - do you know who does curation over Compat.jl to discuss this (and settle what exactly and how should go into PR).

Alternatively I would also be OK if a decision that keepat! is not added to Compat.jl were made. For me it is crucial to have a decision on this topic as I need to know how keepat! should be added to DataFrames.jl. Thank you!

nalimilan commented 2 years ago

I'm not sure there's a main maintainer of Compat.jl so it's not clear who should decide. But if you make a PR I guess it would be accepted as it seems similar to features that have previously been added.

The fact that keepat! has been implemented in two PRs shouldn't be a problem since AFAICT both have been included in Julia 1.7.0 (via a backport for the second one: https://github.com/JuliaLang/julia/commit/b97fde6fc8f3be93a032754326048b58953fdc89), and we probably don't care about supporting 1.7.0-DEV versions. (In theory when VERSION is between the two PRs, it should be enough to add a method Base.keepat!(a::AbstractVector, m::AbstractVector{Bool}), but it's not worth it IMO.)

bkamins commented 2 years ago

OK - I have opened #770 then. Thank you!