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

@views annotation affect how elements are printed #1893

Closed
ronisbr opened this issue Jul 21, 2019 · 5 comments
Closed

@views annotation affect how elements are printed #1893

ronisbr opened this issue Jul 21, 2019 · 5 comments

Comments

@ronisbr
Copy link
Member

ronisbr commented Jul 21, 2019

Hi guys!

The print behavior of string in DataFrame changes if the @view annotation is used. For example, consider the following code:

using DataFrames

df = DataFrame(:a => "Teste 1", :b => "Teste 2")

for i = 1:2
    println(df[1,i])
end

@views for i = 1:2
    println(df[1,i])
end

The output is:

Teste 1
Teste 2
"Teste 1"
"Teste 2"

I would expect that the behavior would not change if the @views annotation is used. This is caused because of the type data[1,i] when @views is used. In the first case it is String whereas in the second it is SubArray{String,0,Array{String,1},Tuple{Int64},true}.

I found this bug by analyzing the cause of this one: ronisbr/PrettyTables.jl#16

@bkamins
Copy link
Member

bkamins commented Jul 21, 2019

The quick solution: please use getindex instead of [ syntax.

Regarding the core of the problem I will have to discuss it with @mbauman (I hope I will get a chance to do this).

Essentially Base now disallows the following:

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

julia> x[1] .= 10
ERROR: MethodError: no method matching copyto!(::Int64, ::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{0},Tuple{},typeof(identity),Tuple{Int64}})

In data frames we allow it:

julia> df = DataFrame(zeros(2,2))
2×2 DataFrame
│ Row │ x1      │ x2      │
│     │ Float64 │ Float64 │
├─────┼─────────┼─────────┤
│ 1   │ 0.0     │ 0.0     │
│ 2   │ 0.0     │ 0.0     │

julia> df[1,1] .= 100
0-dimensional view(::Array{Float64,1}, 1) with eltype Float64:
100.0

julia> df
2×2 DataFrame
│ Row │ x1      │ x2      │
│     │ Float64 │ Float64 │
├─────┼─────────┼─────────┤
│ 1   │ 100.0   │ 0.0     │
│ 2   │ 0.0     │ 0.0     │

I am not clear which is better (but this is related to other 0-dimensional broadcasting cases that were uncovered here #1890).

The fix is easy and quick to do (so hopefully all can be cleaned up during JuliaCon and a patch release tagged), but I want to make an informed decision before I move forward.

@ronisbr
Copy link
Member Author

ronisbr commented Jul 21, 2019

Thanks @bkamins ! In fact, in my specific case, it turns out that I do not need @views at all. But I though it might be interesting to report since we usually expect that such annotation does not have any effect on the code when printing.

ronisbr added a commit to ronisbr/PrettyTables.jl that referenced this issue Jul 21, 2019
The loop that creates the strings to be printed was using `@views`
annotation. This introduced the issue #16, which is related to
JuliaData/DataFrames.jl#1893. However, it turns out that such
annotation was not providing any improvements in the performance.
Thus, it was removed.

Closes #16
@bkamins
Copy link
Member

bkamins commented Jul 21, 2019

Thank you for reporting this. Actually this is a crucial part of consistency with Base regarding broadcasting for DataFrames.jl (i.e. how do we handle 0-dimensional containers and if/and how to materialize them). These are corner cases, but when you have hundreds of users they pop up quite fast 😄.

@bkamins
Copy link
Member

bkamins commented Jul 21, 2019

Actually we should change the DataFrames.jl behavior to mimic what happens here:

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

julia> x[1] .= 100
3-element Array{Int64,1}:
 100
 100
 100

julia> x
1-element Array{Array{Int64,1},1}:
 [100, 100, 100]

I will add it to #1890.

@bkamins
Copy link
Member

bkamins commented Jul 26, 2019

fixed on 0.19.1

@bkamins bkamins closed this as completed Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants