rafaqz / DimensionalData.jl

Named dimensions and indexing for julia arrays and other data
https://rafaqz.github.io/DimensionalData.jl/stable/
MIT License
271 stars 38 forks source link

Inconsistency of `set` #478

Closed sethaxen closed 1 year ago

sethaxen commented 1 year ago

When the 2nd argument of set is a Dimension, set is inconsistent about whether it updates the name of the dimension or not:

julia> lu = LookupArrays.Sampled(1:10);

julia> set(Dim{:foo}(lu), :bar)  # (✓) changes name (✓) keeps lookup
Dim{:bar} Sampled{Int64} AutoOrder AutoSpan AutoSampling
wrapping: 1:10

julia> set(Dim{:foo}(lu), Dim{:bar})  # (✓) changes name (✓) keeps lookup
Dim{:bar} Sampled{Int64} AutoOrder AutoSpan AutoSampling
wrapping: 1:10

julia> set(Dim{:foo}(lu), Dim{:bar}(2:11))  # (✓) changes name (✓) changes lookup
Dim{:bar} Sampled{Int64} ForwardOrdered Regular AutoSampling
wrapping: 2:11

julia> set(Dim{:foo}(lu), Dim{:bar}(LookupArrays.Sampled(2:11)))  # (✗) changes name (✓) changes lookup
Dim{:foo} Sampled{Int64} ForwardOrdered Regular AutoSampling
wrapping: 2:11

julia> set(Dim{:foo}(lu), Dim{:bar}(lu))   # (✗) changes name (✓) lookup unchanged
Dim{:foo} Sampled{Int64} ForwardOrdered Regular AutoSampling
wrapping: 1:10

As a bonus, the following cases just error instead of updating the dimension name:

julia> set(Dim{:foo}(), :bar)
ERROR: ArgumentError: Can not set any fields of Colon to Colon
Stacktrace:
 [1] _cantseterror(a::Function, b::Function)
   @ DimensionalData.Dimensions.LookupArrays ~/projects/DimensionalData.jl/src/LookupArrays/set.jl:99
 [2] _set(A::Function, x::Function)
   @ DimensionalData.Dimensions.LookupArrays ~/projects/DimensionalData.jl/src/LookupArrays/set.jl:96
 [3] _set(dim::Dim{:foo, Colon}, newdim::Dim{:bar, Colon})
   @ DimensionalData.Dimensions ~/projects/DimensionalData.jl/src/Dimensions/set.jl:24
 [4] _set(dim::Dim{:foo, Colon}, key::Symbol)
   @ DimensionalData.Dimensions ~/projects/DimensionalData.jl/src/Dimensions/set.jl:27
 [5] set(dim::Dim{:foo, Colon}, x::Symbol)
   @ DimensionalData.Dimensions ~/projects/DimensionalData.jl/src/Dimensions/set.jl:3
 [6] top-level scope
   @ REPL[506]:1

julia> set(Dim{:foo}(1:10), :bar)
ERROR: ArgumentError: Can not set any fields of UnitRange{Int64} to Colon
Stacktrace:
 [1] _cantseterror(a::UnitRange{Int64}, b::Function)
   @ DimensionalData.Dimensions.LookupArrays ~/projects/DimensionalData.jl/src/LookupArrays/set.jl:99
 [2] _set(A::UnitRange{Int64}, x::Function)
   @ DimensionalData.Dimensions.LookupArrays ~/projects/DimensionalData.jl/src/LookupArrays/set.jl:96
 [3] _set(dim::Dim{:foo, UnitRange{Int64}}, newdim::Dim{:bar, Colon})
   @ DimensionalData.Dimensions ~/projects/DimensionalData.jl/src/Dimensions/set.jl:24
 [4] _set(dim::Dim{:foo, UnitRange{Int64}}, key::Symbol)
   @ DimensionalData.Dimensions ~/projects/DimensionalData.jl/src/Dimensions/set.jl:27
 [5] set(dim::Dim{:foo, UnitRange{Int64}}, x::Symbol)
   @ DimensionalData.Dimensions ~/projects/DimensionalData.jl/src/Dimensions/set.jl:3
 [6] top-level scope
   @ REPL[507]:1
rafaqz commented 1 year ago

Aarg yeah. set is so complicated, there are some bugs I have been trying to ignore

sethaxen commented 1 year ago

Are there issues for other bugs? Or other bugs you could list here? It would be good to know what to work around. I came across these for #477, and it may be easier to fix them in set than add workarounds for them in replacedims.

rafaqz commented 1 year ago

I dont think so? I just remember noticing something similar a few months ago during another PR and have the feeling I didnt get around to making the issue.

Definately fixing set is bettter.