DimensionalData.jl icon indicating copy to clipboard operation
DimensionalData.jl copied to clipboard

separate `set` and `unsafe_set` so that users can't end up with broken lookups

Open felixcremer opened this issue 1 year ago • 4 comments

When I try to use At("Red") on a ForwardOrdered dimension this fails. I got the Dimension by using set on a Band dimension from GDAL but I can also reproduce this with plain DD calls. When I use


julia> dd = DimArray(rand(2,3), (Dim{:colors}(["Red", "Green"]), X(1:3)))
2×3 DimArray{Float64,2} with dimensions: 
  Dim{:colors} Categorical{String} String["Red", "Green"] ReverseOrdered,
  X Sampled{Int64} 1:3 ForwardOrdered Regular Points
           1         2         3
  "Red"    0.492407  0.360465  0.149189
  "Green"  0.215122  0.595209  0.985611


julia> dd1 = set(dd, X=>Dim{:Colors}(["Red", "Blue", "Green"]))
2×3 DimArray{Float64,2} with dimensions: 
  Dim{:colors} Categorical{String} String["Red", "Green"] ReverseOrdered,
  Dim{:Colors} Sampled{String} String["Red", "Blue", "Green"] ForwardOrdered Regular Points
            "Red"     "Blue"    "Green"
  "Red"    0.492407  0.360465  0.149189
  "Green"  0.215122  0.595209  0.985611

julia> dd1[Colors=At("Red")]
selval = "Red"
i = 3
ERROR: ArgumentError: Red not found in ["Red", "Blue", "Green"]
Stacktrace:
  [1] _selvalnotfound(lookup::DimensionalData.Dimensions.LookupArrays.Sampled{…}, selval::String)
    @ DimensionalData.Dimensions.LookupArrays ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:202
  [2] _selnotfound_or_nothing(err::DimensionalData.Dimensions.LookupArrays._True, lookup::DimensionalData.Dimensions.LookupArrays.Sampled{…}, selval::String)
    @ DimensionalData.Dimensions.LookupArrays ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:200
  [3] at(::DimensionalData.Dimensions.LookupArrays.ForwardOrdered, ::DimensionalData.Dimensions.LookupArrays.Regular{…}, lookup::DimensionalData.Dimensions.LookupArrays.Sampled{…}, selval::String, atol::Nothing, rtol::Nothing; err::DimensionalData.Dimensions.LookupArrays._True)
    @ DimensionalData.Dimensions.LookupArrays ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:183
  [4] at
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:159 [inlined]
  [5] #at#24
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:143 [inlined]
  [6] at
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:142 [inlined]
  [7] selectindices
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/LookupArrays/selector.jl:120 [inlined]
  [8] _dims2indices
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/Dimensions/indexing.jl:114 [inlined]
  [9] macro expansion
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/Dimensions/indexing.jl:56 [inlined]
 [10] _dims2indices
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/Dimensions/indexing.jl:56 [inlined]
 [11] dims2indices
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/Dimensions/indexing.jl:51 [inlined]
 [12] dims2indices
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/Dimensions/indexing.jl:34 [inlined]
 [13] #getindex#58
    @ ~/Documents/PyramidScheme/dev/DimensionalData/src/array/indexing.jl:47 [inlined]
 [14] top-level scope
    @ REPL[56]:1
Some type information was truncated. Use `show(err)` to see complete types.

felixcremer avatar Feb 15 '24 15:02 felixcremer

Oh, yeah... weird, I can reproduce.

lazarusA avatar Feb 15 '24 15:02 lazarusA

Its not forward ordered tho, so searchsorted wont work.

Probably set should error

rafaqz avatar Feb 15 '24 17:02 rafaqz

I expected to fully replace the Dimension so that it has an order that fits to the values. If set errors how would you change a dimension?

felixcremer avatar Feb 15 '24 23:02 felixcremer

Maybe we need to let you set both in the same call so there is never an incorrect state.

The problem with auto-detecting the order in set is it would make every change to the lookup values type-unstable. Normally set is a compile-time op or close to it so you can use it freely in hot paths. It would be unfortunate to lose that. format does these things but its never expected to be type stable and only happens in user called constructors.

But I've been thinking we may need to split set into unsafe_set and set. unsafe_set wouldn't check anything that would lead to type instability, and would be safe for use in hot paths, while set would be safe for users to use in the REPL and would avoid problems like this one.

rafaqz avatar Feb 29 '24 00:02 rafaqz