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

fix repeat and some other minor issues #2648

Merged
merged 4 commits into from
Mar 10, 2021
Merged

fix repeat and some other minor issues #2648

merged 4 commits into from
Mar 10, 2021

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Mar 8, 2021

Some fixes after JuliaLang/julia#31829.
Mainly restricting the signature of repeat.

Additionally one dangling get to unwrap for CategoricalArrays.jl compatibility I missed earlier.

@mbauman

The JuliaLang/julia#31829 broke the following thing in Julia Base that was not covered by tests (fortunately we had appropriate tests in DataFrames.jl):

julia> x = [1,2,3]
3-element Vector{Int64}:
 1
 2
 3

julia> repeat(x, inner=1,outer=1)
3-element Vector{Int64}:
 1
 2
 3

julia> repeat(x, inner=1,outer=false) # it works but I think it should be an error
Int64[]

julia> repeat(x, inner=false,outer=1) # previously it worked
ERROR: ArgumentError: invalid index: false of type Bool

@bkamins
Copy link
Member Author

bkamins commented Mar 8, 2021

Actually maybe Bool in repeat should be allowed as it is not an indexing context and it is clear what will be the result of the operation. Let us wait what @mbauman comments for Julia Base.

@bkamins
Copy link
Member Author

bkamins commented Mar 8, 2021

As commented above - I have thought about it and I think that we should accept Bool as in this context it should be treated as Integer.

Comment on lines 296 to 298
if VERSION > v"1.6"
@test_throws ArgumentError SubDataFrame(sdf, Integer[true, true, true], :)
end
Copy link
Member

Choose a reason for hiding this comment

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

And what happens before 1.6? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Before (and including 1.6) 1.7 it works - see the old test that got removed.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, shouldn't this be tested too?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - I have reverted it (although it was kept mainly to keep track of the bug in Julia Base).

A follow up PR in Julia Base that should be fixed is JuliaLang/julia#39962 (I am pinging you as it seem no one commented on it).

@bkamins bkamins merged commit 9d87bcf into main Mar 10, 2021
@bkamins bkamins deleted the bk/fix_tests branch March 10, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants