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

Type stability of Tables.matrix(::NamedTuple) #339

Closed
MilesCranmer opened this issue Aug 5, 2023 · 3 comments · Fixed by #340
Closed

Type stability of Tables.matrix(::NamedTuple) #339

MilesCranmer opened this issue Aug 5, 2023 · 3 comments · Fixed by #340

Comments

@MilesCranmer
Copy link
Contributor

I'm descending some type instability issues in my package and realized that Tables.matrix(::NamedTuple) is not type stable. Would this be feasible to implement?

For example:

julia> using Tables

julia> X = (x=randn(32), y=randn(32));

julia> typeof(Tables.matrix(X))
Matrix{Float64} (alias for Array{Float64, 2})

julia> @code_warntype Tables.matrix(X)
MethodInstance for Tables.matrix(::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}})
  from matrix(table; transpose) @ Tables ~/.julia/packages/Tables/AcRIE/src/matrix.jl:84
Arguments
  #self#::Core.Const(Tables.matrix)
  table::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}
Body::Matrix
1%1 = Tables.:(var"#matrix#85")(false, #self#, table)::Matrix
└──      return %1

So the type inference is not able to infer that this is a Matrix{Float64}, only that it is a Matrix{Any}, despite the method call being specialized to a NamedTuple with Vector{Float64} for columns.

Version info:

Julia Version 1.10.0-beta1
Commit 6616549950e (2023-07-25 17:43 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin22.4.0)
  CPU: 8 × Apple M1 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, apple-m1)
  Threads: 8 on 6 virtual cores
Environment:
  JULIA_FORMATTER_SO = /Users/mcranmer/julia_formatter.so
  JULIA_NUM_THREADS = auto
  JULIA_EDITOR = code
@MilesCranmer
Copy link
Contributor Author

It looks like the source of this instability is here (via Cthulhu)

 84 function matrix(table::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}; transpose::Bool::Bool=false)::Matrix
 85     cols::Tables.Columns{@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}} = Columns::Type{Tables.Columns}(table::@NamedTuple{x::Vector{Float64}, y::Vector{Float64}})::Tables.Columns{@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}}
 86     types::Tuple{Vararg{DataType}} = schema(cols::Tables.Columns{@NamedTuple{x::Vector{Float64}, y::Vector{Float64}}})::Tables.Schema{(:x, :y), Tuple{Float64, Float64}}.types::Tuple{Vararg{DataType}}

So the schema(cols).types is being inferred as a Tuple{Vararg{DataType}} rather than a Tuple{Float64,Float64}.

@MilesCranmer
Copy link
Contributor Author

MilesCranmer commented Aug 5, 2023

I think the issue is that this function:

function Base.getproperty(sch::Schema{names, types}, field::Symbol) where {names, types}
    if field === :names
        return names === nothing ? getfield(sch, :storednames) : names
    elseif field === :types
        T = getfield(sch, :storedtypes)
        return types === nothing ? (T !== nothing ? T : nothing) : Tuple(fieldtype(types, i) for i = 1:fieldcount(types))
    else
        throw(ArgumentError("unsupported property for Tables.Schema"))
    end
end

does not have a type stable return value.

@MilesCranmer
Copy link
Contributor Author

Fixed in #340!

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

Successfully merging a pull request may close this issue.

1 participant