Improve corner cases of deleteat! by bkamins · Pull Request #42144 · JuliaLang/julia

bkamins · 2021-09-07T11:59:58Z

This addresses issues discussed in #42065 (comment) for deleteat!.

base/array.jl

bkamins · 2021-09-07T13:35:06Z

What problems this PR tries to solve:

julia> deleteat!([1, 2, 3], true)
2-element Vector{Int64}:
 2
 3

julia> deleteat!([1, 2, 3], true:true)
2-element Vector{Int64}:
 2
 3

julia> deleteat!(trues(3), [true, false, false])
ERROR: ArgumentError: indices must be unique and sorted

julia> deleteat!(trues(3), [UInt8(1)])
ERROR: MethodError: no method matching copy_chunks!(::Vector{UInt64}, ::UInt8, ::Vector{UInt64}, ::Int64, ::Int64)

base/array.jl

bkamins · 2021-09-07T13:36:30Z

I will add tests and NEWS.md entry if the proposed changes would be acceptable.

bkamins · 2021-09-07T17:45:01Z

base/bitarray.jl

+    if length(B) != length(inds)
+        throw(ArgumentError("length of Bool indices inds must match the length of B"))
+    end
+    return deleteat!(B, findall(inds))


clearly this can be made faster, but will lead to some code duplication. Should I improve things here?

Seems fine for now. Would it be faster to reuse the code for Vector (at least avoiding the allocation)? The Vector method should work if _deleteend! is just replaced with resize!.

no - it would not be fully efficient. I will implement an efficient method and you will judge.

dkarrasch

Just stumbled across the typo.

base/array.jl

base/bitarray.jl

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

base/array.jl

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>

bkamins · 2021-09-16T21:54:44Z

I have not made PRs to Julia for some time. Could someone please guide me through which CI jobs are running the actual tests of the functionality? (as there seem to be many jobs doing some specific testing) Thank you!

nalimilan · 2021-09-17T20:12:45Z

I have not made PRs to Julia for some time. Could someone please guide me through which CI jobs are running the actual tests of the functionality? (as there seem to be many jobs doing some specific testing) Thank you!

AFAIK "package" jobs only build Julia, "tester" jobs run the tests. Not sure about others.

JeffBezanson · 2021-09-17T20:48:50Z

Looks great, just need to fix the tests.

bkamins · 2021-09-18T07:06:06Z

base/bitarray.jl


    (p, s) = y
    checkbounds(B, p)
+    p isa Bool && throw(ArgumentError("invalid index $p of type Bool"))


We can throw an error here (and not just warn about deprecation), because earlier it already threw an error as a side effect of lack of conversion of p to Int where needed later. (same below)

The current behavior is:

julia> deleteat!(falses(5), Any[true]) ERROR: MethodError: no method matching copy_chunks!(::Vector{UInt64}, ::Bool, ::Vector{UInt64}, ::Int64, ::Int64) julia> deleteat!(falses(5), Any[1, true]) ERROR: ArgumentError: indices must be unique and sorted

Now in both cases we will throw throw(ArgumentError("invalid index $p of type Bool"))

bkamins · 2021-09-18T07:07:27Z

base/bitarray.jl

 end

 function splice!(B::BitVector, i::Integer)
+    # TODO: after deprecation remove the four lines below


I have also added the checks for Bool to splice! as they are done in non-BitVector case:

julia> splice!([1,2,3], true) ERROR: ArgumentError: invalid index: true of type Bool julia> splice!([1,2,3], true, [4,5]) ERROR: ArgumentError: invalid index: true of type Bool

bkamins · 2021-09-18T07:09:23Z

just need to fix the tests.

I have fixed. Actually the test was correct, but it was failing because there was an additional issue with lacking conversion to Int in the old code that required fixing. Now it should be good for a review. I have added some comments in places that are non-obvious without running the code.

bkamins · 2021-09-18T08:38:06Z

re-triggering CI

bkamins · 2021-09-18T19:51:57Z

A single CI error seems unrelated, so this should be good for a final review.

base/array.jl

base/bitarray.jl

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de> (cherry picked from commit deb3fac)

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

bkamins · 2023-03-13T20:05:10Z

Could this PR be backported to 1.6 release? see JuliaData/DataFrames.jl#3300

bkamins added 2 commits September 7, 2021 13:59

Improve corner cases of deleteat!

30096c3

Update array.jl

1b24441

bkamins commented Sep 7, 2021

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

bkamins added 2 commits September 7, 2021 15:26

Update base/array.jl

bf4bec0

Update bitarray.jl

9444ed4

bkamins commented Sep 7, 2021

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

Update base/array.jl

e630812

bkamins marked this pull request as ready for review September 7, 2021 13:36

bkamins commented Sep 7, 2021

View reviewed changes

dkarrasch reviewed Sep 8, 2021

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

base/array.jl Outdated Show resolved Hide resolved

base/bitarray.jl Outdated Show resolved Hide resolved

base/bitarray.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

89ef38a

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

JeffBezanson reviewed Sep 16, 2021

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

bkamins and others added 2 commits September 16, 2021 19:07

Update base/array.jl

3533746

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>

implement faster deleteat! and initial tests

3adf111

bkamins added 2 commits September 17, 2021 00:10

add tests

a74d908

add tests

5c9fbb3

JeffBezanson approved these changes Sep 17, 2021

View reviewed changes

bkamins added 2 commits September 18, 2021 00:09

improve implementation in corner cases

9d9c09e

code fixes

ba04859

bkamins commented Sep 18, 2021

View reviewed changes

bkamins closed this Sep 18, 2021

bkamins reopened this Sep 18, 2021

JeffBezanson reviewed Sep 21, 2021

View reviewed changes

base/array.jl Outdated Show resolved Hide resolved

base/array.jl Outdated Show resolved Hide resolved

base/bitarray.jl Outdated Show resolved Hide resolved

base/bitarray.jl Outdated Show resolved Hide resolved

base/bitarray.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

bd3fbbf

JeffBezanson added backport 1.7 merge me PR is reviewed. Merge when all tests are passing labels Sep 21, 2021

JeffBezanson closed this Sep 22, 2021

JeffBezanson reopened this Sep 22, 2021

JeffBezanson merged commit deb3fac into JuliaLang:master Sep 22, 2021

bkamins deleted the patch-29 branch September 22, 2021 19:16

KristofferC mentioned this pull request Sep 23, 2021

release-1.7: Backports for 1.7-rc2 #42255

Merged

84 tasks

JeffBezanson mentioned this pull request Sep 23, 2021

keepat! and deleteat!: inconsistencies and possible bugs #42065

Closed

DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Sep 24, 2021

KristofferC pushed a commit that referenced this pull request Sep 28, 2021

Improve corner cases of deleteat! (#42144)

03cee00

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

KristofferC pushed a commit that referenced this pull request Oct 5, 2021

Improve corner cases of deleteat! (#42144)

76b30f0

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de> (cherry picked from commit deb3fac)

KristofferC removed the backport 1.7 label Oct 22, 2021

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022

Improve corner cases of deleteat! (JuliaLang#42144)

04be679

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022

Improve corner cases of deleteat! (JuliaLang#42144)

a38f689

Co-authored-by: Daniel Karrasch <daniel.karrasch@posteo.de>

bkamins mentioned this pull request Mar 13, 2023

dropmissing! fails when one column in DataFrame is of type bitvector JuliaData/DataFrames.jl#3300

Open

Uh oh!

Conversation

bkamins commented Sep 7, 2021

Uh oh!

Uh oh!

bkamins commented Sep 7, 2021

Uh oh!

Uh oh!

bkamins commented Sep 7, 2021

Uh oh!

bkamins Sep 7, 2021

Choose a reason for hiding this comment

Uh oh!

JeffBezanson Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

bkamins Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

dkarrasch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkamins commented Sep 16, 2021

Uh oh!

nalimilan commented Sep 17, 2021

Uh oh!

JeffBezanson commented Sep 17, 2021

Uh oh!

bkamins Sep 18, 2021

Choose a reason for hiding this comment

Uh oh!

bkamins Sep 18, 2021

Choose a reason for hiding this comment

Uh oh!

bkamins commented Sep 18, 2021

Uh oh!

bkamins commented Sep 18, 2021

Uh oh!

bkamins commented Sep 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkamins commented Mar 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants