[Containers] add support for Base.getindex(::Container; kwargs...) #3237

odow · 2023-02-23T22:29:18Z

Closes #1291

There are still a few issues to work through:

documentation
DenseAxisView
Slicing needs to preserve names
What if broadcasting mixes names? Answer: we take the first set of names. We can't force equal names because that would be breaking.

codecov · 2023-02-24T00:22:18Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (faa7290) 98.12% compared to head (9f99aa5) 98.13%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3237      +/-   ##
==========================================
+ Coverage   98.12%   98.13%   +0.01%     
==========================================
  Files          34       34              
  Lines        4740     4835      +95     
==========================================
+ Hits         4651     4745      +94     
- Misses         89       90       +1

Impacted Files	Coverage Δ
src/Containers/DenseAxisArray.jl	`96.37% <100.00%> (+0.51%)`	⬆️
src/Containers/SparseAxisArray.jl	`99.35% <100.00%> (+0.91%)`	⬆️
src/Containers/container.jl	`94.33% <100.00%> (-3.49%)`	⬇️
src/Containers/macro.jl	`100.00% <100.00%> (ø)`
src/JuMP.jl	`97.01% <100.00%> (+0.13%)`	⬆️
src/constraints.jl	`96.56% <100.00%> (+0.05%)`	⬆️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

metab0t · 2023-02-24T12:02:44Z

Maybe DenseAxisView should also be adapted to support @view A[i=1, j=:]?

Furthermore, what do you think about the following convenient features:

Remove the restrictions for orders of kwargs A[i=1, j=2] == A[j=2, i=1]
Automatic inserting : for missing dimensions A[i=2:3] == A[i=2:3, j=:]

odow · 2023-02-26T04:41:40Z

Maybe DenseAxisView should also be adapted to support @view A[i=1, j=:]?

👍

what do you think about the following convenient features:

I think they're a little too magical. This syntax is opt-in and more verbose, so it will be used by people looking for correctness and to make the code more readable. Enforcing an order of arguments helps clarify that that, and so does forcing arguments like j = :.

docs/src/manual/containers.md

blegat · 2023-02-27T10:28:43Z

We don't have a good answer for fixing the usability issue of a user doing

model = Model()
@variable(model, x[i=1:2, j = 1:3])
x[i = 1, j = 3] # errors as `x` is a `Matrix`.

We have many users that don't care about which type of container is returned by macro and it this kind of erros would be very confusing.

What we need to do is to return a DenseAxisArray if there are names in the macro.
If a user really need to have an Array and not a DenseAxisArray, this user cares about container so we can ask him to do

@variable(model, x[i=1:2, j = 1:3], lower_bound = i + j, requested_container = Array)

However, that is breaking so maybe we need to wait for JuMP v2.0 for this PR ?

odow · 2023-02-27T19:32:40Z

It's confusing, but because it's opt-in, the user can also opt-in to container = DenseAxisArray.

I think changing the default is a bad trade-off. Giving everyone a heavier, slower, more complicated data structure just so some people don't get confused when using kwarg indexing?

blegat · 2023-02-28T13:36:32Z

It's confusing, but because it's opt-in, the user can also opt-in to container = DenseAxisArray.

What is opt-in ? If the user uses keyword indexing with an array and it gets an error, he didn't opt-in for anything and we won't understand a thing

I think changing the default is a bad trade-off. Giving everyone a heavier, slower, more complicated data structure just so some people don't get confused when using kwarg indexing

It shouldn't be heavier if every axis is Base.OneTo thanks to #2524

odow · 2023-02-28T19:38:44Z

What is opt-in ?

Choosing to use kwargs instead of args. args works the same everywhere. kwargs doesn't. That's for the user to figure out.

We're also not going to change the default before JuMP 2.0, so this is a moot discussion.

blegat · 2023-03-01T10:02:56Z

We're also not going to change the default before JuMP 2.0, so this is a moot discussion.

Why do you mean by the default ? The one we advertise in the docs ?

Choosing to use kwargs instead of args. args works the same everywhere. kwargs doesn't. That's for the user to figure out.

If keyword argument indexing works, the user will just use it without realizing he has opt-in to anything. I don't see how that fixes the usability issue.

odow · 2023-03-01T20:04:38Z

Why do you mean by the default ?

The type created by @variable(model, x[i=1:2, j=1:2])

I don't see how that fixes the usability issue.

It doesn't. But it means they can choose a trade-off. Positional indexing works the same everywhere. Kwarg indexing works for Dense and SparseAxisArray.

It's not like you can only use positional with Array and kwarg with {Dense,Sparse}AxisArray.

I also think this discussion is somewhat academic. Let's get it out there and see if people complain.

blegat · 2023-03-02T08:29:08Z

I also think this discussion is somewhat academic

I think it's the opposite. A company will never add a feature knowing that it will cause confusion to beginners for which the answer is "they have to know better the inner working for JuMP".

Let's get it out there and see if people complain.

If they do, we don't have any other option than breaking changes so not sure I want to take that risk just for fixing an issue raised 5 years ago with no activity for 5 years.

odow · 2023-03-02T08:49:25Z

@mlubin you're going to need to take a look at this one.

mlubin

My proposal is to make this explicitly opt in, i.e., disabled unless you pass keyword_indexing=True to @variable or @constraint. This way we can provide a useful error message when keyword_indexing=True and the container is Array.
Let's see if the feature gets used and if anyone likes it, then we can decide what to break for the next major release.

mlubin · 2023-03-04T18:51:04Z

src/Containers/DenseAxisArray.jl

+end
+
+function Base.getindex(x::DenseAxisArrayView, args...; kwargs...)
+    if !isempty(kwargs)


getindex is performance sensitive. Is there any impact from this extra !isempty(kwargs) in the case of all positional arguments?

Nope. It gets compiled away:

shell> git status On branch od/container-kwarg-getindex Your branch is up to date with 'origin/od/container-kwarg-getindex'. nothing to commit, working tree clean julia> using JuMP julia> using BenchmarkTools julia> model = Model() A JuMP Model Feasibility problem with: Variables: 0 Model mode: AUTOMATIC CachingOptimizer state: NO_OPTIMIZER Solver name: No optimizer attached. julia> @variable(model, x[i=2:3, j=4:5]) 2-dimensional DenseAxisArray{VariableRef,2,...} with index sets: Dimension 1, 2:3 Dimension 2, 4:5 And data, a 2×2 Matrix{VariableRef}: x[2,4] x[2,5] x[3,4] x[3,5] julia> @btime getindex($x, 2, 4) 3.466 ns (0 allocations: 0 bytes) x[2,4] julia> exit() (base) oscar@Oscars-MBP JuMP % git checkout master Switched to branch 'master' Your branch is up to date with 'origin/master'. (base) oscar@Oscars-MBP JuMP % julia --project=. _ _ _ _(_)_ | Documentation: https://docs.julialang.org (_) | (_) (_) | _ _ _| |_ __ _ | Type "?" for help, "]?" for Pkg help. | | | | | | |/ _` | | | | |_| | | | (_| | | Version 1.6.7 (2022-07-19) _/ |\__'_|_|_|\__'_| | Official https://julialang.org/ release |__/ | julia> using JuMP [ Info: Precompiling JuMP [4076af6c-e467-56ae-b986-b466b2749572] julia> using BenchmarkTools julia> model = Model() A JuMP Model Feasibility problem with: Variables: 0 Model mode: AUTOMATIC CachingOptimizer state: NO_OPTIMIZER Solver name: No optimizer attached. julia> @variable(model, x[i=2:3, j=4:5]) 2-dimensional DenseAxisArray{VariableRef,2,...} with index sets: Dimension 1, 2:3 Dimension 2, 4:5 And data, a 2×2 Matrix{VariableRef}: x[2,4] x[2,5] x[3,4] x[3,5] julia> @btime getindex($x, 2, 4) 3.725 ns (0 allocations: 0 bytes) x[2,4]

There's still room to improve the kwarg option, but let's settle on the syntax/opt-in stuff first:

julia> @btime getindex($x; i = 2, j = 4) 33.646 ns (1 allocation: 32 bytes) x[2,4]

Is kwarg indexing fundamentally 10x slower than positional indexing? If that's true we might want to reconsider the syntax.

No, if you change the method to a singular getindex(x; kwargs...) it comes down to 12ns and 0 allocations, but then I needed to add a bunch of additional methods to avoid ambiguities. Left as-is for now while we sort the syntax.

Keyword indexing in general does have a slight performance hit though.

Keyword indexing in general does have a slight performance hit though.

In fact, keyword indexing can be as fast as Cartesian indexing if the tuple of names is encoded into the customized array type, but it is obviously not an option for JuMP now.

blegat · 2023-03-04T19:12:03Z

My proposal is to make this explicitly opt in, i.e., disabled unless you pass keyword_indexing=True to @variable or @constraint. This way we can provide a useful error message when keyword_indexing=True and the container is Array.
Let's see if the feature gets used and if anyone likes it, then we can decide what to break for the next major release.

That would resolve the usability issue. Maybe this option can be at the level of the model ?

mlubin · 2023-03-05T14:03:25Z

Maybe this option can be at the level of the model ?

If it's at the level of the model, then does this mean we print a warning or error for every Array container in the model?

odow · 2023-03-06T00:10:59Z

I looked into the flag option, and it's actually pretty complicated to do with a model-level flag, because the value of the flag needs to be available at macro-expansion time. so @variable(model, x[I=1:2], enable_keyword_indexing = true) works, but some model-level thing doesn't.

One option could be some global constant and require users to opt-in with JuMP.Containers.enable_keyword_indexing() = true.

But in general, I think the option is over-kill. Is it really too much to ask a user who chooses to use keyword indexing to understand the difference between an Array and a DenseAxisArray? It's a shame that we can't provide a nice error message, but the default isn't that bad, and I can explain in the intro documentation:

julia> model = Model();

julia> @variable(model, x[i=1:2, j=1:2])
2×2 Matrix{VariableRef}:
 x[1,1]  x[1,2]
 x[2,1]  x[2,2]

julia> @variable(model, y[i=1:2, j=1:2], container = DenseAxisArray)
2-dimensional DenseAxisArray{VariableRef,2,...} with index sets:
    Dimension 1, Base.OneTo(2)
    Dimension 2, Base.OneTo(2)
And data, a 2×2 Matrix{VariableRef}:
 y[1,1]  y[1,2]
 y[2,1]  y[2,2]

julia> x[i=1, j=1]
ERROR: MethodError: no method matching getindex(::Matrix{VariableRef}; i=1, j=1)
Closest candidates are:
  getindex(::Array, ::Int64) at array.jl:805 got unsupported keyword arguments "i", "j"
  getindex(::Array, ::Int64, ::Int64, ::Int64...) at array.jl:806 got unsupported keyword arguments "i", "j"
  getindex(::Array, ::UnitRange{Int64}) at array.jl:809 got unsupported keyword arguments "i", "j"
  ...
Stacktrace:
 [1] top-level scope
   @ REPL[8]:1

julia> y[i=1, j=1]
y[1,1]

mlubin · 2023-03-06T02:24:08Z

@variable(model, x[i=1:2, j=1:2])
What about generating a warning here because the index variables are unused and have no effect?
This should catch a fair chunk of cases where users might be confused by the lack of kwarg indexing.

odow · 2023-03-06T02:37:27Z

It seems weird to throw an error there, but not if @variable(model, x[i=1:2, j=1:2] <= i). And it doesn't cover containers of expressions or constraints, which are also liable for errors.

And this is still premised on the fact that users will be confused, which I'm not sure they will be.

If we get complaints, then I think it's something that can be resolved with documentation. Not with warnings or enable_keyword_indexing_and_I_understand_the_caveats(model) functions.

Spitballing other ideas:

We could make something like x[NamedIndex(i = 2, j = 2)] work. It's explicitly opt-in, and we could give a nice error for Array.

blegat · 2023-03-06T08:23:25Z

What about generating a warning here because the index variables are unused and have no effect?

They could be used for readability/self-documented code as well.

We could make something like x[NamedIndex(i = 2, j = 2)] work. It's explicitly opt-in, and we could give a nice error for Array.

I find the keyword option less heavy syntax.

Is it really too much to ask a user who chooses to use keyword indexing to understand the difference between an Array and a DenseAxisArray?

Is this meant for container or keyword_indexing keyword ?

Thinking more about it, users might still find keyword indexing being used in the middle of JuMP code and then trying it and be confused without knowing about the keyword_indexing so I'm not sure it's improving it so much compared to the additional annoyance of having to pass this keyword argument every time.

Returning a DenseAxisArray for @variable(model, [i=1:2, j=1:2]) seems like the only right fix to me but it's breaking :/

odow · 2023-03-06T18:02:50Z

We seem to be stalemating. To summarize where we're at:

We want x[; kwargs...] syntax
The syntax doesn't work for Array, and we can't provide a nice error message
We can't change the default container for x[i=1:2] to DenseAxisArray because that is breaking

The options are:

Release as-is and wait to see if users find it confusing that they can use keyword indexing only with DenseAxisArray and SparseAxisArray
Have some enable_keyword_indexing(model) opt-in
Throw some warning
Don't merge

The warning is hard, because it'd be hard to distinguish x[i=1:2] >= i from x[i=1:2] >= 0, and we might end up throwing a warning when the user never intended to use kwarg indexing, or not throw a warning when they did.

The opt-in is also hard because it would require the macro code to depend on the run-time setting in model. Although we've done this before with set_string_names_on_creation, this is a little more complicated to implement because it's in the Container submodule. I'll take another stab at implementing.

If that doesn't work, we're either left with 1 or 4, which probably means 4 if we don't agree.

mlubin · 2023-03-07T02:03:48Z

It seems weird to throw an error there, but not if @variable(model, x[i=1:2, j=1:2] <= i)

IMO it's not at all weird to warn when there are unused variables.

And it doesn't cover containers of expressions or constraints, which are also liable for errors.

Agreed.

If a user really need to have an Array and not a DenseAxisArray, this user cares about container so we can ask him to do @variable(model, x[i=1:2, j = 1:3], lower_bound = i + j, requested_container = Array) However, that is breaking so maybe we need to wait for JuMP v2.0 for this PR ?

This change seems very disruptive for people who are already happily using Array containers. If we do this we should be confident about how useful the keyword indexing is. It's hard to have this confidence before there are people using it in the wild and giving us feedback. So, I'm in favor of the more explicit opt-in version. It's ok with me if it's a flag to the macro. We don't need a complicated model-level flag. If the feedback is, "this feature is great but it's too annoying to opt in", then maybe we'd make it the default and deal with the fallout.

odow · 2023-03-07T02:26:50Z

It's ok with me if it's a flag to the macro. We don't need a complicated model-level flag.

What about a global flag via Containers.enable_keyword_indexing() = true? That's a lot simpler to implement than a model- or macro-specific flag. (And a lot less tedious for people to try out.)

mlubin · 2023-03-07T12:55:14Z

A global flag means this feature can't be used in library code.

odow · 2023-03-07T19:46:54Z

I guess I was imagining that this flag was going to be used in user-level code only

Okay. Let me do the macro- and model-level flag...

odow · 2023-03-07T20:56:11Z

The demo is

julia> using JuMP

julia> model = Model();

julia> @variable(model, x[i = 2:3])
1-dimensional DenseAxisArray{VariableRef,1,...} with index sets:
    Dimension 1, 2:3
And data, a 2-element Vector{VariableRef}:
 x[2]
 x[3]

julia> x[i = 2]
ERROR: Keyword indexing is disabled. To enable, pass `enable_keyword_indexing = true` to the `Containers.@container` macro, or call `JuMP.enable_container_keyword_indexing(model, true)` before calling any JuMP macros like `@variable`.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] _check_keyword_indexing_allowed(#unused#::Nothing)
   @ JuMP.Containers ~/.julia/dev/JuMP/src/Containers/Containers.jl:31
 [3] #_kwargs_to_args#17
   @ ~/.julia/dev/JuMP/src/Containers/DenseAxisArray.jl:356 [inlined]
 [4] #getindex#20
   @ ~/.julia/dev/JuMP/src/Containers/DenseAxisArray.jl:375 [inlined]
 [5] top-level scope
   @ REPL[5]:1

julia> model = Model();

julia> enable_container_keyword_indexing(model, true)

julia> @variable(model, x[i = 2:3])
1-dimensional DenseAxisArray{VariableRef,1,...} with index sets:
    Dimension 1, 2:3
And data, a 2-element Vector{VariableRef}:
 x[2]
 x[3]

julia> x[i = 2]
x[2]

src/JuMP.jl

src/Containers/SparseAxisArray.jl

odow · 2023-03-26T23:07:32Z

Performance issue is still unresolved, #3237 (comment), but I'll fix that in a separate PR. This PR is already quite complicated.

blegat · 2023-03-27T20:04:20Z

src/JuMP.jl

+    )
+end
+
+function Base.getindex(x::Array{<:AbstractJuMPScalar}; kwargs...)


Isn't that called when you do getindex(x) without any keyword argument ? There should be at least one

What does it mean to call getindex(x)?

Even without JuMP:

julia> x = rand(3) 3-element Vector{Float64}: 0.8542832661650948 0.36511705612410084 0.12183318054742132 julia> x[] ERROR: BoundsError: attempt to access 3-element Vector{Float64} at index [] Stacktrace: [1] throw_boundserror(A::Vector{Float64}, I::Tuple{}) @ Base ./abstractarray.jl:651 [2] checkbounds @ ./abstractarray.jl:616 [inlined] [3] _getindex @ ./abstractarray.jl:1196 [inlined] [4] getindex(::Vector{Float64}) @ Base ./abstractarray.jl:1170 [5] top-level scope @ REPL[451]:1

But yeah, I guess we could throw a bounds error if there are no kwargs and we get dispatched here.

odow changed the title ~~WIP: [Containers] add support for Base.getindex(::Container; kwargs...)~~ [Containers] add support for Base.getindex(::Container; kwargs...) Feb 24, 2023

odow commented Feb 27, 2023

View reviewed changes

docs/src/manual/containers.md Outdated Show resolved Hide resolved

blegat mentioned this pull request Feb 27, 2023

Style fix: requestedcontainer -> requested_container #3247

Merged

odow mentioned this pull request Feb 27, 2023

Prep for v1.9.0 #3251

Merged

14 tasks

odow force-pushed the od/container-kwarg-getindex branch from a5e5ce1 to 3bab327 Compare February 28, 2023 02:21

mlubin reviewed Mar 4, 2023

View reviewed changes

odow force-pushed the od/container-kwarg-getindex branch from 1698ebe to f208e55 Compare March 6, 2023 00:19

odow and others added 13 commits March 24, 2023 09:09

More docs

47ba2e0

Add ENABLE_KEYWORD_INDEXING

a9d5f54

Fix formatting

e92df40

Add enable_container_keyword_indexing

4468531

Fix docs

b3a3f9b

Apply suggestions from code review

6ddbe48

Apply suggestions from code review

e54ebd4

Improve docstring

5af91f9

Update

405fd7a

Update

da504e0

Fix scope issue

bc7e213

Fix

f105fe6

Revert to an error for getindex

dbfdca7

odow force-pushed the od/container-kwarg-getindex branch from 4f3ac10 to dbfdca7 Compare March 23, 2023 20:54

odow removed the Status: Needs developer call This should be discussed on a monthly developer call label Mar 23, 2023

mlubin approved these changes Mar 26, 2023

View reviewed changes

odow commented Mar 26, 2023

View reviewed changes

src/JuMP.jl Outdated Show resolved Hide resolved

odow commented Mar 26, 2023

View reviewed changes

src/Containers/SparseAxisArray.jl Outdated Show resolved Hide resolved

odow commented Mar 26, 2023

View reviewed changes

src/Containers/SparseAxisArray.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

4a348fc

odow mentioned this pull request Mar 26, 2023

Prep for v1.10.0 #3304

Merged

2 tasks

odow requested a review from blegat March 26, 2023 23:07

blegat reviewed Mar 27, 2023

View reviewed changes

Update error for []

9f99aa5

blegat approved these changes Mar 29, 2023

View reviewed changes

odow merged commit 0778238 into master Mar 29, 2023

odow deleted the od/container-kwarg-getindex branch March 29, 2023 17:48

This was referenced Apr 5, 2023

Don't access vector with [] jump-dev/DiffOpt.jl#238

Merged

Broken getindex with no arguments #3313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Containers] add support for Base.getindex(::Container; kwargs...) #3237

[Containers] add support for Base.getindex(::Container; kwargs...) #3237

odow commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

metab0t commented Feb 24, 2023

odow commented Feb 26, 2023 •

edited

Loading

blegat commented Feb 27, 2023

odow commented Feb 27, 2023

blegat commented Feb 28, 2023

odow commented Feb 28, 2023

blegat commented Mar 1, 2023

odow commented Mar 1, 2023

blegat commented Mar 2, 2023

odow commented Mar 2, 2023

mlubin left a comment •

edited

Loading

mlubin Mar 4, 2023

odow Mar 6, 2023

mlubin Mar 6, 2023

odow Mar 6, 2023

metab0t Mar 6, 2023

blegat commented Mar 4, 2023

mlubin commented Mar 5, 2023

odow commented Mar 6, 2023

mlubin commented Mar 6, 2023 •

edited

Loading

odow commented Mar 6, 2023

blegat commented Mar 6, 2023 •

edited

Loading

odow commented Mar 6, 2023

mlubin commented Mar 7, 2023

odow commented Mar 7, 2023

mlubin commented Mar 7, 2023

odow commented Mar 7, 2023

odow commented Mar 7, 2023

odow commented Mar 26, 2023

blegat Mar 27, 2023

odow Mar 27, 2023

odow Mar 27, 2023

[Containers] add support for Base.getindex(::Container; kwargs...) #3237

[Containers] add support for Base.getindex(::Container; kwargs...) #3237

Conversation

odow commented Feb 23, 2023 • edited Loading

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

metab0t commented Feb 24, 2023

odow commented Feb 26, 2023 • edited Loading

blegat commented Feb 27, 2023

odow commented Feb 27, 2023

blegat commented Feb 28, 2023

odow commented Feb 28, 2023

blegat commented Mar 1, 2023

odow commented Mar 1, 2023

blegat commented Mar 2, 2023

odow commented Mar 2, 2023

mlubin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blegat commented Mar 4, 2023

mlubin commented Mar 5, 2023

odow commented Mar 6, 2023

mlubin commented Mar 6, 2023 • edited Loading

odow commented Mar 6, 2023

Spitballing other ideas:

blegat commented Mar 6, 2023 • edited Loading

odow commented Mar 6, 2023

mlubin commented Mar 7, 2023

odow commented Mar 7, 2023

mlubin commented Mar 7, 2023

odow commented Mar 7, 2023

odow commented Mar 7, 2023

odow commented Mar 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odow commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

odow commented Feb 26, 2023 •

edited

Loading

mlubin left a comment •

edited

Loading

mlubin commented Mar 6, 2023 •

edited

Loading

blegat commented Mar 6, 2023 •

edited

Loading