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

cat is not type-inferrable #500

Open
sethaxen opened this issue Jul 25, 2023 · 4 comments
Open

cat is not type-inferrable #500

sethaxen opened this issue Jul 25, 2023 · 4 comments

Comments

@sethaxen
Copy link
Collaborator

sethaxen commented Jul 25, 2023

It seems that when using cat, Julia cannot infer the types of the resulting dimensions:

julia> using DimensionalData, Test

julia> foo(x, y) = cat(x, y; dims=Dim{:foo}([:c, :d]));

julia> da = DimArray(randn(3), Dim{:a})
3-element DimArray{Float64,1} with dimensions: Dim{:a}
 1   0.54291
 2  -0.0289045
 3   0.526266

julia> foo(da, da)
3×2 DimArray{Float64,2} with dimensions: 
  Dim{:a},
  Dim{:foo} Categorical{Symbol} Symbol[:c, :d] ForwardOrdered
   :c          :d
  0.54291     0.54291
 -0.0289045  -0.0289045
  0.526266    0.526266

julia> @inferred foo(da, da)
ERROR: return type DimArray{Float64, 2, Tuple{Dim{:a, DimensionalData.Dimensions.LookupArrays.NoLookup{Base.OneTo{Int64}}}, Dim{:foo, DimensionalData.Dimensions.LookupArrays.Categorical{Symbol, Vector{Symbol}, DimensionalData.Dimensions.LookupArrays.ForwardOrdered, DimensionalData.Dimensions.LookupArrays.NoMetadata}}}, Tuple{}, Matrix{Float64}, DimensionalData.NoName, DimensionalData.Dimensions.LookupArrays.NoMetadata} does not match inferred return type DimArray{Float64, 2, _A, Tuple{}, Matrix{Float64}, DimensionalData.NoName, DimensionalData.Dimensions.LookupArrays.NoMetadata} where _A<:Tuple
@rafaqz
Copy link
Owner

rafaqz commented Jul 25, 2023

Yeah, it doesnt yet know the order or if the lookup is ordered at all.

You can (fully) specify the Categorical lookup manually and it should be type stable.

@rafaqz
Copy link
Owner

rafaqz commented Jul 27, 2023

Yes its fine if you specify Categorical with the order keyword:

using DimensionalData.LookupArrays
bar(x, y) = cat(x, y; dims=Dim{:foo}(Categorical([:c, :d]; order=ForwardOrdered())))

julia> @inferred bar(da, da)
3×2 DimArray{Float64,2} with dimensions: 
  Dim{:a},
  Dim{:foo} Categorical{Symbol} Symbol[:c, :d] ForwardOrdered
   :c         :d
  1.6825     1.6825
  0.216932   0.216932
 -0.66003   -0.66003

@sethaxen
Copy link
Collaborator Author

@rafaqz it seems on v0.25 even specifying the lookup does not cause cat to be type-inferrable:

julia> using DimensionalData, Test

julia> using DimensionalData.LookupArrays

julia> bar(x, y) = cat(x, y; dims=Dim{:foo}(Categorical([:c, :d]; order=ForwardOrdered())))
bar (generic function with 1 method)

julia> da = DimArray(randn(3), Dim{:a})
3-element DimArray{Float64,1} with dimensions: Dim{:a}
 1  -0.214735
 2   0.395572
 3  -0.388922

julia> @inferred bar(da, da)
ERROR: return type DimArray{Float64, 2, Tuple{Dim{:a, NoLookup{Base.OneTo{Int64}}}, Dim{:foo, Categorical{Symbol, Vector{Symbol}, ForwardOrdered, NoMetadata}}}, Tuple{}, Matrix{Float64}, DimensionalData.NoName, NoMetadata} does not match inferred return type Any

@rafaqz
Copy link
Owner

rafaqz commented Sep 26, 2023

Making cat type stable when allowing it to still work on runtime-validated incorrect dims is pretty hard.

Maybe we need to separate out method dispatch for this specific case so its not mixed into a method with multiple return types.

A PR would help, I've spent too much time working on cat lately 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants