Skip to content

Conversation

@odow
Copy link
Member

@odow odow commented Mar 10, 2022

We'd discussed this before, #2523, and I thought I fixed it, but I missed a scoping issue.

Raised on discourse: https://discourse.julialang.org/t/using-alternative-array-container-types-in-jump/77707

@odow odow added Type: Bug Category: Containers Related to the Containers submodule labels Mar 10, 2022
@mlubin
Copy link
Member

mlubin commented Mar 10, 2022

Do we document the interface that external containers need to satisfy for this to work?

@odow
Copy link
Member Author

odow commented Mar 10, 2022

No, this is essentially undocumented because it's not something we really want to advertise: https://discourse.julialang.org/t/using-alternative-array-container-types-in-jump/77707/3.

We should instead encourage people to create their own containers outside of JuMP, rather than extending things in this way.

@odow
Copy link
Member Author

odow commented Mar 10, 2022

@mlubin
Copy link
Member

mlubin commented Mar 10, 2022

Why support it at all if we don't want to document it?

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #2916 (97468ce) into master (d8763fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2916   +/-   ##
=======================================
  Coverage   95.20%   95.20%           
=======================================
  Files          43       43           
  Lines        5774     5777    +3     
=======================================
+ Hits         5497     5500    +3     
  Misses        277      277           
Impacted Files Coverage Δ
src/Containers/container.jl 97.72% <ø> (ø)
src/Containers/macro.jl 100.00% <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 d8763fd...97468ce. Read the comment docs.

@odow
Copy link
Member Author

odow commented Mar 10, 2022

I'd be okay documenting this in the Developer/Extensions section.

We introduced it with the view to unifying the named-array landscape in Julia, #2523, but we have some unique requirements that I don't know if it makes sense to do so.

The main distinction is that some package developers might want to extend things, but general users probably shouldn't.

@mlubin
Copy link
Member

mlubin commented Mar 10, 2022

Documenting it in the Developer/Extensions section would be fine. What error message will users get if they try a container that hasn't implemented the Containers method?

@odow
Copy link
Member Author

odow commented Mar 10, 2022

It's currently (after this PR)

julia> struct Foo end;

julia> @variable(model, x[S], Bin, container = Foo)
ERROR: Unable to build a container with the provided type Foo. Implement `Containers.container`.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] container(#unused#::Function, #unused#::JuMP.Containers.VectorizedProductIterator{Tuple{Vector{String}}}, D::Type)
   @ JuMP.Containers ~/.julia/dev/JuMP/src/Containers/container.jl:176
 [3] macro expansion
   @ ~/.julia/dev/JuMP/src/macros.jl:142 [inlined]
 [4] top-level scope
   @ REPL[21]:1

TODOs

  • Add documentation
  • Improve error message

@mlubin
Copy link
Member

mlubin commented Mar 10, 2022

Ok, that's already a decent error message as is. I was worried about MethodErrors.

@odow odow merged commit 29d6758 into master Mar 13, 2022
@odow odow deleted the od/container-scope branch March 13, 2022 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Containers Related to the Containers submodule Type: Bug

Development

Successfully merging this pull request may close these issues.

3 participants