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

Allow pairs in constructors #530

Closed
ParadaCarleton opened this issue Sep 5, 2023 · 5 comments · Fixed by #602
Closed

Allow pairs in constructors #530

ParadaCarleton opened this issue Sep 5, 2023 · 5 comments · Fixed by #602

Comments

@ParadaCarleton
Copy link
Contributor

ParadaCarleton commented Sep 5, 2023

Some constructors that feel like they should work, but don't ATM:

julia> z = randn(3, 3) |> x->x' * x |> x->DimArray(x, (ax1=(:x, :y, :z), ax2=(:x, :y, :z)))
ERROR: MethodError: no method matching format(::Tuple{Symbol, Symbol, Symbol}, ::Type{Dim{:ax1}}, ::Base.OneTo{Int64})

Closest candidates are:
  format(::DimensionalData.Dimensions.LookupArrays.LookupArray, ::Type, ::AbstractRange)
   @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/Dimensions/format.jl:39
  format(::DimensionalData.Dimensions.LookupArrays.NoLookup, ::Type, ::Any, ::AbstractRange)
   @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/Dimensions/format.jl:43
  format(::DimensionalData.Dimensions.LookupArrays.Categorical, ::Type, ::Any, ::AbstractRange)
   @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/Dimensions/format.jl:60
  ...

Stacktrace:
   [1-5]  internal
       @ DimensionalData.Dimensions, DimensionalData
     [6] DimArray
       @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/array/array.jl:326 [inlined]
     [7] #36
       @ Main ./REPL[47]:1 [inlined]
     [8] |>
       @ Base ./operators.jl:915 [inlined]
     [9] #35
       @ Main ./REPL[47]:1 [inlined]
    [10] |>(x::Matrix{Float64}, f::var"#35#37")
       @ Base ./operators.jl:915
Use `err` to retrieve the full stack trace.

Can't use pairs either:

julia> z = randn(3, 3) |> x->x' * x |> x->DimArray(x, (:axis1=>[:x, :y, :z], :axis2=>[:x, :y, :z]))
ERROR: MethodError: no method matching _format(::Pair{Symbol, Vector{Symbol}}, ::Base.OneTo{Int64})

Closest candidates are:
  _format(::Symbol, ::AbstractRange)
   @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/Dimensions/format.jl:29
  _format(::DimensionalData.Dimensions.LookupArrays.Irregular{DimensionalData.Dimensions.LookupArrays.AutoBounds}, ::Any, ::Any)
   @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/Dimensions/format.jl:99
  _format(::Type{D}, ::AbstractRange) where D<:DimensionalData.Dimensions.Dimension
   @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/Dimensions/format.jl:30
  ...

Stacktrace:
   [1-4]  internal
       @ DimensionalData.Dimensions, DimensionalData
     [5] DimArray(data::Matrix{Float64}, dims::Tuple{Pair{Symbol, Vector{Symbol}}, Pair{Symbol, Vector{Symbol}}})
       @ DimensionalData ~/.julia/packages/DimensionalData/4TpBG/src/array/array.jl:326
     [6] (::var"#58#60")(x::Matrix{Float64})
       @ Main ./REPL[72]:1
     [7] |>(x::Matrix{Float64}, f::var"#58#60")
       @ Base ./operators.jl:915
     [8] (::var"#57#59")(x::Matrix{Float64})
       @ Main ./REPL[72]:1
     [9] |>(x::Matrix{Float64}, f::var"#57#59")
       @ Base ./operators.jl:915
Use `err` to retrieve the full stack trace.
@rafaqz
Copy link
Owner

rafaqz commented Sep 5, 2023

The lookups very much need to be AbstractArray:

(ax1=(:x, :y, :z), ax2=(:x, :y, :z))

So we would have to collect them in format. (probably should have a better error otherwise). Any reason not to just make them arrays from the start?

This looks like it should work though... I remember even making this work at one point writing it and deciding not to just to keep it simpler:

(:axis1=>[:x, :y, :z], :axis2=>[:x, :y, :z])

Not sure how to weight that up

@ParadaCarleton
Copy link
Contributor Author

So we would have to collect them in format. Any reason not to just make them arrays from the start?

No reason for using a tuple, I just keep forgetting whether it's a tuple or an array 😅 but I assume if this is a problem for me, I'm probably not the only one.

@rafaqz
Copy link
Owner

rafaqz commented Sep 6, 2023

Probably needs a better error catching Tuple and whatever else then.

@ParadaCarleton
Copy link
Contributor Author

That would work, but is there a problem with just collecting the Tuple?

@rafaqz
Copy link
Owner

rafaqz commented Sep 7, 2023

The problem for me is that if it works, people will use it, and its confusing to see a Tuple working as an AbstractArray

@rafaqz rafaqz changed the title Generalize Constructors Allow pairs in constructors Jan 31, 2024
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.

2 participants