Remove unnecessary convert methods

[bkamins]

We define a bunch of convert methods in DataFrames.jl that are most likely never used (as instead just type constructors are used). Having these convert methods causes invalidations, so we can drop them.

This is breaking but I think we can make these breaking changes in 1.0 release. (@KristofferC - do you have some estimate how much latency this causes? - thank you!)

[KristofferC]

What I saw from my SnoopCompiling was:

 inserting convert(::Type{Tuple}, key::DataFrames.GroupKey) in DataFrames at /Users/kristoffercarlsson/.julia/packages/DataFrames/oQ5c7/src/groupeddataframe/groupeddataframe.jl:507 invalidated:
   mt_backedges:  1: signature Tuple{typeof(convert), Type{Tuple}, Any} triggered MethodInstance for
...
                 38: signature Tuple{typeof(convert), Type{Tuple}, Any} triggered MethodInstance for (::Type{AbstractPlotting.LineSegments{_A}} where _A)(::AbstractPlotting.Axis3D{_A} where _A, ::Any, ::AbstractPlotting.Attributes, ::Tuple{Observables.Observable{Vector{GeometryBasics.Point{3, Float32}}}}, ::Any, ::Vector{AbstractPlotting.AbstractPlot}) (12 children)
                 39: signature Tuple{typeof(convert), Type{Tuple}, Any} triggered MethodInstance for (::Type{AbstractPlotting.Text{_A}} where _A)(::AbstractPlotting.Annotations{Tuple{Vector{Tuple{String, GeometryBasics.Point{2, Float32}}}}}, ::Any, ::AbstractPlotting.Attributes, ::Any, ::Any, ::Vector{AbstractPlotting.AbstractPlot}) (17 children)
                 40: signature Tuple{typeof(convert), Type{Tuple}, Any} triggered MethodInstance for (::Type{AbstractPlotting.Annotations{_A}} where _A)(::AbstractPlotting.Axis3D{_A} where _A, ::Any, ::AbstractPlotting.Attributes, ::Tuple{Observables.Observable{Vector{String}}, Observables.Observable{Vector{GeometryBasics.Point{3, Float32}}}}, ::Any, ::Vector{AbstractPlotting.AbstractPlot}) (114 children)

So there is some bad interaction between AbstractPlotting and DataFrames. So it isn't a huge problem but depending on the level of breakage it might make sense to fix it anyway.

I don't have any concrete numbers for the level of latency this causes. The similar on CategoricalArrays is ~10x bigger than this as a refrence.

[bkamins]

Digging in the history it seems that @ChrisRackauckas uses (or used) this kind of conversion methods.
Most likely it was in a generic code that was even unaware of DataFrames.jl existence.

So, while it seems to make sense to remove unnecessary convert in favor of explicit constructor call it seems that legacy code actually uses convert. @nalimilan - what do you think?

[bkamins]

Let me try to give some structure to what we discuss. First the manual:

since convert can be called implicitly, its methods are restricted to cases that are considered "safe" or "unsurprising". convert will only convert between types that represent the same basic kind of thing (e.g. different representations of numbers, or different string encodings). It is also usually lossless; converting a value to a different type and back again should result in the exact same value.

Now what convert methods do we define in DataFrames.jl with my assessment:

• convert(::Type{Matrix}, df::AbstractDataFrame): not OK; looses information
• convert(::Type{Matrix{T}}, df::AbstractDataFrame) where T: not OK; looses information
• convert(::Type{Array}, df::AbstractDataFrame): not OK; looses information
• convert(::Type{Array{T}}, df::AbstractDataFrame) where {T}: not OK; looses information
• convert(::Type{DataFrame}, d::AbstractDict): OK, but is it really needed to have?
• convert(::Type{DataFrame}, sdf::SubDataFrame): OK, should stay
• convert(::Type{Vector}, dfr::DataFrameRow): not OK; looses information
• convert(::Type{Vector{T}}, dfr::DataFrameRow) where T: not OK; looses information
• convert(::Type{Array}, dfr::DataFrameRow): not OK; looses information
• convert(::Type{Array{T}}, dfr::DataFrameRow) where {T}: not OK; looses information
• convert(::Type{NamedTuple}, dfr::DataFrameRow): OK, should stay
• convert(::Type{Tuple}, dfr::DataFrameRow): not OK; looses information
• convert(::Type{NamedTuple}, key::GroupKey): OK, should stay
• convert(::Type{Tuple}, key::GroupKey): not OK; looses information
• convert(::Type{Vector}, key::GroupKey): not OK; looses information
• convert(::Type{Vector{T}}, key::GroupKey) where T: not OK; looses information
• convert(::Type{Array}, key::GroupKey): not OK; looses information
• convert(::Type{Array{T}}, key::GroupKey) where {T}: not OK; looses information

Now let us check this against:

The following language constructs call convert:

• Assigning to an array converts to the array's element type.
• Assigning to a field of an object converts to the declared type of the field.
• Constructing an object with new converts to the object's declared field types.
• Assigning to a variable with a declared type (e.g. local x::T) converts to that type.
• A function with a declared return type converts its return value to that type.
• Passing a value to ccall converts it to the corresponding argument type.

If I recall correctly in the past convert was more heavily used by Julia Base, but now these use cases are really mostly undesired for DataFrames.jl objects. The only conversions I would leave are:

• convert(::Type{DataFrame}, sdf::SubDataFrame): OK, should stay
• convert(::Type{NamedTuple}, dfr::DataFrameRow): OK, should stay
• convert(::Type{NamedTuple}, key::GroupKey): OK, should stay

I would deprecate the rest of conversion methods as they are more likely to confuse people rather than be useful.

@nalimilan - what is your take here?

[pdeffebach]

• convert(::Type{DataFrame}, d::AbstractDict): OK, but is it really needed to have?

Not needed, especially after Tables.

[nalimilan]

I agree with your list. I think a good rule of thumb for a convert method to make sense is that isequal(convert(T, x), x) and/or isequal(convert(typeof(x), convert(T, x)), x) should hold.

[bkamins]

OK - the only question then is if we remove the unnecessary convert now or deprecate them and remove in 2.0?

[ChrisRackauckas]

Is it really that weird to want a matrix from a file? The problem is that file readers many times give a DataFrame that you don't want. I think it's odd to single me out as the only one who wants to convert to matrices when I'm definitely not the only one doing it (and this is clear from all of the Slack questions that came up about it before it was put back).

[bkamins]

I agree you might want to create a matrix from a data frame and we want to keep it. We are just considering removing a convert method. So Matrix(df) would still work, but convert(Matrix, df) would not work.

However, since this is an important decision could you please comment if you would use convert or only Matrix constructor? Thank you!

[ChrisRackauckas]

I would use either, that's fine. I thought you meant there would be no way to convert to a matrix. But just curious, is there a precedent of another type that allows Matrix(x) but not convert(Matrix,x)?

[bkamins]

I do not think for Matrix, but in general these differences are present in Base in many places.

This is the most relevant comparison (as you drop names from NamedTuple so this is essentially the same case we have with a DataFrame and Matrix):

julia> convert(Tuple, (a=1, b=2))
ERROR: MethodError: Cannot `convert` an object of type NamedTuple{(:a, :b), Tuple{Int64, Int64}} to an object of type Tuple

julia> Tuple((a=1, b=2))
(1, 2)

and here are some more examples:

julia> convert(String, :a)
ERROR: MethodError: Cannot `convert` an object of type Symbol to an object of type String

julia> String(:a)
"a"

julia> convert(String, ['a'])
ERROR: MethodError: Cannot `convert` an object of type Vector{Char} to an object of type String

julia> String(['a'])
"a"

julia> @enum Fruit apple=1 orange=2 kiwi=3

julia> convert(Int, apple)
ERROR: MethodError: Cannot `convert` an object of type Fruit to an object of type Int64

julia> Int(apple)
1

I guess convert was used much more in the past but currently its use case got much narrower (per the parts of the documentation I cited above).

[ChrisRackauckas]

I see, makes sense then.