Allow `_AxisLookup` to fallback to Dict `getindex` #3028

nickrobinson251 · 2022-07-26T15:54:11Z

Allows e.g. looking up String key with another AbstractString
The use-case here is when a DenseAxisArray is created with InlineString keys but where this is an implementation detail and we want to be able to index with a regular String

Before:

julia> using JuMP, InlineStrings

julia> daa = JuMP.Containers.DenseAxisArray(reshape(1:6, 2, :), 1:2, ["a","b","c"])
2-dimensional DenseAxisArray{Int64,2,...} with index sets:
    Dimension 1, 1:2
    Dimension 2, ["a", "b", "c"]
And data, a 2×3 Matrix{Int64}:
 1  3  5
 2  4  6

julia> daa[1, "b"]
3

julia> daa[1, InlineString("b")]
ERROR: KeyError: key "b" not found
Stacktrace:
 [1] getindex(#unused#::JuMP.Containers._AxisLookup{Dict{String, Int64}}, key::String1)
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:18
 [2] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{String1}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:309
 [3] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Int64, String1}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:308
 [4] to_index(A::JuMP.Containers.DenseAxisArray{Int64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, idx::Tuple{Int64, String1})
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:318
 [5] getindex(::JuMP.Containers.DenseAxisArray{Int64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, ::Int64, ::String1)
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:325
 [6] top-level scope
   @ REPL[33]:1

After (this PR):

julia> daa[1, InlineString("b")]
3

julia> daa2 = JuMP.Containers.DenseAxisArray(reshape(1:6, 2, :), 1:2, InlineString.(["a","b","c"]));

julia> daa2[1, "b"]
3

- Allows e.g. looking up `String` key with another `AbstractString`

codecov · 2022-07-26T22:23:28Z

Codecov Report

Merging #3028 (c85a2f3) into master (1844b21) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3028   +/-   ##
=======================================
  Coverage   96.13%   96.13%           
=======================================
  Files          32       32           
  Lines        4143     4143           
=======================================
  Hits         3983     3983           
  Misses        160      160

Impacted Files	Coverage Δ
src/Containers/DenseAxisArray.jl	`90.36% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1844b21...c85a2f3. Read the comment docs.

test/Containers/DenseAxisArray.jl

odow · 2022-07-27T00:25:49Z

So this PR removes tests for

JuMP.jl/src/Containers/DenseAxisArray.jl

Line 18 in 1844b21

Base.getindex(::_AxisLookup, key) = throw(KeyError(key))

but it still passes, so I guess we end up throwing the same key error.

What happens if you try indexing now with a Symbol or something else?

Perhaps it is preferable to add an explicit AbstractString method so we're clear in why we're doing this.

nickrobinson251 · 2022-07-27T08:52:24Z

guess we end up throwing the same key error.

Yes, that's right, now getindex for Dict throws the KeyError

Before:

julia> D = DenseAxisArray([5.0 6.0; 7.0 8.0], 2:3, ["a", "b"])
2-dimensional DenseAxisArray{Float64,2,...} with index sets:
    Dimension 1, 2:3
    Dimension 2, ["a", "b"]
And data, a 2×2 Matrix{Float64}:
 5.0  6.0
 7.0  8.0

julia> D[2, :a]
ERROR: KeyError: key :a not found
Stacktrace:
 [1] getindex(#unused#::JuMP.Containers._AxisLookup{Dict{String, Int64}}, key::Symbol)
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:18
 [2] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Symbol}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:309
 [3] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Int64, Symbol}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:308
 [4] to_index(A::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, idx::Tuple{Int64, Symbol})
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:318
 [5] getindex(::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, ::Int64, ::Symbol)
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:325
 [6] top-level scope
   @ REPL[37]:1
   
julia> D[2, ["a", :b]]
ERROR: KeyError: key Any["a", :b] not found
Stacktrace:
 [1] getindex(#unused#::JuMP.Containers._AxisLookup{Dict{String, Int64}}, key::Vector{Any})
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:18
 [2] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Vector{Any}}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:309
 [3] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Int64, Vector{Any}}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:308
 [4] to_index(A::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, idx::Tuple{Int64, Vector{Any}})
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:318
 [5] getindex(::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, ::Int64, ::Vector{Any})
   @ JuMP.Containers ~/.julia/packages/JuMP/Y4piv/src/Containers/DenseAxisArray.jl:325
 [6] top-level scope
   @ REPL[38]:1

After (this PR):

julia> D = DenseAxisArray([5.0 6.0; 7.0 8.0], 2:3, ["a", "b"]);

julia> D[2, :a]
ERROR: KeyError: key :a not found
Stacktrace:
 [1] getindex(h::Dict{String, Int64}, key::Symbol)
   @ Base ./dict.jl:498
 [2] getindex(x::JuMP.Containers._AxisLookup{Dict{String, Int64}}, key::Symbol)
   @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:51
 [3] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Symbol}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:309
 [4] _getindex_recurse(data::Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}, keys::Tuple{Int64, Symbol}, condition::JuMP.Containers.var"#10#12")
   @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:308
 [5] to_index(A::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, idx::Tuple{Int64, Symbol})
   @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:318
 [6] getindex(::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, ::Int64, ::Symbol)
   @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:325
 [7] top-level scope
   @ REPL[9]:1
   
julia> D[2, ["a", :b]]
ERROR: KeyError: key :b not found
Stacktrace:
  [1] getindex(h::Dict{String, Int64}, key::Symbol)
    @ Base ./dict.jl:498
  [2] getindex(x::JuMP.Containers._AxisLookup{Dict{String, Int64}}, key::Symbol)
    @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:51
  [3] (::JuMP.Containers.var"#1#2"{JuMP.Containers._AxisLookup{Dict{String, Int64}}})(key::Symbol)
    @ JuMP.Containers ./none:0
  [4] iterate
    @ ./generator.jl:47 [inlined]
  [5] collect_to!(dest::Vector{Int64}, itr::Base.Generator{Vector{Any}, JuMP.Containers.var"#1#2"{JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, offs::Int64, st::Int64)
    @ Base ./array.jl:845
  [6] collect_to_with_first!(dest::Vector{Int64}, v1::Int64, itr::Base.Generator{Vector{Any}, JuMP.Containers.var"#1#2"{JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, st::Int64)
    @ Base ./array.jl:823
  [7] collect(itr::Base.Generator{Vector{Any}, JuMP.Containers.var"#1#2"{JuMP.Containers._AxisLookup{Dict{String, Int64}}}})
    @ Base ./array.jl:797
  [8] getindex
    @ ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:58 [inlined]
  [9] _getindex_recurse
    @ ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:309 [inlined]
 [10] _getindex_recurse
    @ ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:308 [inlined]
 [11] to_index
    @ ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:318 [inlined]
 [12] getindex(::DenseAxisArray{Float64, 2, Tuple{UnitRange{Int64}, Vector{String}}, Tuple{JuMP.Containers._AxisLookup{Tuple{Int64, Int64}}, JuMP.Containers._AxisLookup{Dict{String, Int64}}}}, ::Int64, ::Vector{Any})
    @ JuMP.Containers ~/repos/JuMP.jl/src/Containers/DenseAxisArray.jl:325
 [13] top-level scope
    @ REPL[10]:1

The message is different in the vector case, but I think actually more accurate now: before it might suggest we were looking for the (single) key ["a", :b] but actually we were looking for (each of) the key "a" and the key :b.

Perhaps it is preferable to add an explicit AbstractString method so we're clear in why we're doing this.

We could do that if you prefer. I didn't do that because it'd be more code, and also because it's conceivable that other types will be purposefully implemented to be hash-equal (like InlineStrings are with Strings). Essentially I presumed that _AxisLookup was meant to have the same lookup behaviour as a Dict.

first time contributing here so please just let me know what if anything you'd like changed :)

nickrobinson251 · 2022-07-27T09:03:31Z

this PR removes tests for

Sorry about that. Since I was only adding tests I didn't expect coverage to drop, but you're right that current tests, on _AxisLookup{Dict{...}}, now take a different path to a KeyError. I've added a commit to test that other code path, which is still hit by _AxisLookup{Tuple{...}}.

odow

I guess this is okay. My main concern was users encountering a method error, but the stack trace is already opaque and we still throw the same top-level error.

odow · 2022-07-27T10:14:22Z

Also.

because it's conceivable that other types will be purposefully implemented to be hash-equal

This just feels soooo wrong. I get why in this particular case, but still.

I guess the precedent is

julia> hash(1)
0x5bca7c69b794f8ce

julia> hash(1.0)
0x5bca7c69b794f8ce

So this would allow someone to create a set with integers and index with floats? I think I've seen a couple of posts asking for that over the years. Can't find them on discourse though.

nickrobinson251 · 2022-07-27T10:26:45Z

Thanks!

So this would allow someone to create a set with integers and index with floats?

Yes, i suppose that is true. And likewise allow users to have an index set with e.g. Int32 integers and able to lookup with Int64 integers, which does seem pretty reasonable to me. As with the InlineString case sometime the actual type used is a (performance-related) implementation detail that we don't want users to have to think about i.e. they just write "str" or 123

nickrobinson251 · 2022-07-28T14:58:38Z

If/when this is okay to be merged, I would be grateful if it could also be included in a release fairly soon after if possible, as at Invenia we have quite a lot of places where we need to workaround the current behaviour

odow · 2022-07-28T23:44:12Z

if it could also be included in a release fairly soon after if possible

There's going to be a bit of a delay on that. The next release is going to be 1.2.0, which includes a bunch of massive changes to the NLP interface, and I want to get #2961 in for that. So no firm ETA, but it'd be in a week or two.

nickrobinson251 · 2022-07-29T08:47:39Z

Thanks @odow !

In a week or two.

Okay, good to know - thanks

Allow _AxisLookup to fallback to Dict getindex

64b51e5

- Allows e.g. looking up `String` key with another `AbstractString`

odow reviewed Jul 26, 2022

View reviewed changes

test/Containers/DenseAxisArray.jl Outdated Show resolved Hide resolved

odow reviewed Jul 26, 2022

View reviewed changes

test/Containers/DenseAxisArray.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

2c33414

odow reviewed Jul 26, 2022

View reviewed changes

test/Containers/DenseAxisArray.jl Outdated Show resolved Hide resolved

Update test/Containers/DenseAxisArray.jl

22d1be4

Add test for manually thrown KeyError

c85a2f3

odow approved these changes Jul 27, 2022

View reviewed changes

odow merged commit 82a6efb into jump-dev:master Jul 28, 2022

nickrobinson251 deleted the npr/use-dict-getindex branch July 29, 2022 08:48

BSnelling mentioned this pull request Aug 1, 2022

Reverting change to branches_by_breakpoints invenia/FullNetworkSystems.jl#28

Merged

guimarqu mentioned this pull request Aug 16, 2022

remove Base.getindex of _AxisLookup atoptima/BlockDecomposition.jl#89

Merged

odow mentioned this pull request Sep 1, 2022

[Containers] fix DenseAxisArray with Vector key #3064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `_AxisLookup` to fallback to Dict `getindex` #3028

Allow `_AxisLookup` to fallback to Dict `getindex` #3028

nickrobinson251 commented Jul 26, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

odow commented Jul 27, 2022

nickrobinson251 commented Jul 27, 2022 •

edited

Loading

nickrobinson251 commented Jul 27, 2022 •

edited

Loading

odow left a comment

odow commented Jul 27, 2022

nickrobinson251 commented Jul 27, 2022

nickrobinson251 commented Jul 28, 2022

odow commented Jul 28, 2022

nickrobinson251 commented Jul 29, 2022

Allow _AxisLookup to fallback to Dict getindex #3028

Allow _AxisLookup to fallback to Dict getindex #3028

Conversation

nickrobinson251 commented Jul 26, 2022 • edited Loading

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

odow commented Jul 27, 2022

nickrobinson251 commented Jul 27, 2022 • edited Loading

nickrobinson251 commented Jul 27, 2022 • edited Loading

odow left a comment

Choose a reason for hiding this comment

odow commented Jul 27, 2022

nickrobinson251 commented Jul 27, 2022

nickrobinson251 commented Jul 28, 2022

odow commented Jul 28, 2022

nickrobinson251 commented Jul 29, 2022

Allow `_AxisLookup` to fallback to Dict `getindex` #3028

Allow `_AxisLookup` to fallback to Dict `getindex` #3028

nickrobinson251 commented Jul 26, 2022 •

edited

Loading

codecov bot commented Jul 26, 2022 •

edited

Loading

nickrobinson251 commented Jul 27, 2022 •

edited

Loading

nickrobinson251 commented Jul 27, 2022 •

edited

Loading