Container revamp #1102

mlubin · 2017-09-16T03:01:30Z

Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with AbstractArray. Explicit keyword argument in macro to force container type.

Unfortunately we can't use AxisArrays at the moment because of JuliaArrays/AxisArrays.jl#117. I'm hoping that we can swap AxisArray for JuMPArray before JuMP 1.0/0.19, but for now I'd like to go ahead with this since the basic functionality is there. If we keep JuMPArray, it will need to be polished and performance-tested before the next release.

Broadcast is implemented for JuMPArray, so we can just do resultvalue.(A) when A is an Array or a JuMPArray (but not when it's a Dict).

See commit for list of 11 issues that this PR closes (once merged into master).

Remaining to do for merge:

Add container keyword argument to all macros
Go through issues listed in make JuMPContainers consistent with AbstractArray interface #1047 and check if they are addressed
Better test coverage

codecov · 2017-09-17T20:23:52Z

Codecov Report

Merging #1102 into jump/moi will increase coverage by 0.7%.
The diff coverage is 74.82%.

@@             Coverage Diff             @@
##           jump/moi    #1102     +/-   ##
===========================================
+ Coverage     21.46%   22.17%   +0.7%     
===========================================
  Files            20       20             
  Lines          3647     3571     -76     
===========================================
+ Hits            783      792      +9     
+ Misses         2864     2779     -85

Impacted Files	Coverage Δ
src/operators.jl	`9.42% <ø> (+0.45%)`	⬆️
src/JuMP.jl	`59.18% <ø> (+6.5%)`	⬆️
src/print.jl	`47.72% <ø> (ø)`	⬆️
src/JuMPArray.jl	`65.55% <65.16%> (-14.45%)`	⬇️
src/macros.jl	`74.2% <78.94%> (-1.14%)`	⬇️
src/containers.jl	`97.14% <97.14%> (ø)`

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 76b3430...497083b. Read the comment docs.

codecov · 2017-09-17T20:23:52Z

Codecov Report

Merging #1102 into jump/moi will increase coverage by 1%.
The diff coverage is 78.1%.

@@            Coverage Diff            @@
##           jump/moi    #1102   +/-   ##
=========================================
+ Coverage     21.46%   22.47%   +1%     
=========================================
  Files            20       20           
  Lines          3647     3578   -69     
=========================================
+ Hits            783      804   +21     
+ Misses         2864     2774   -90

Impacted Files	Coverage Δ
src/print.jl	`47.72% <ø> (ø)`	⬆️
src/operators.jl	`9.42% <ø> (+0.45%)`	⬆️
src/JuMP.jl	`59.18% <ø> (+6.5%)`	⬆️
src/JuMPArray.jl	`66.66% <66.3%> (-13.34%)`	⬇️
src/macros.jl	`75.56% <88.09%> (+0.23%)`	⬆️
src/containers.jl	`97.14% <97.14%> (ø)`

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 76b3430...095c030. Read the comment docs.

Replace JuMPDict with Dict. Rewrite JuMPArray to be compatible with AbstractArray. Explicit keyword argument in macro to force container type. Closes #1099 Closes #1047 Closes #417 (collect is now well defined for Array, JuMPArray, and Dict) Closes #833 (`eachindex` and `indices` are defined for JuMPArray) Closes #740 (dot broadcast syntax is now the default, no need to explicitly define vectorized functions) Closes #922 (fixed by checking for duplicates) Closes #933 (corollary: closes #346) Closes #643 (colons work for Array and JuMPArray, obviously not Dict) Closes #730 (end is not supported for JuMPArray) Closes #646 (we now rely on built-in iteration behavior for Dict)

mlubin · 2017-09-19T02:38:46Z

I plan on adding some more tests but otherwise this is ready for review. Note that it closes 11 issues.

joehuchette

Looks good to me. Do you have a sense of the performance relative to the current implementation?

joehuchette · 2017-09-19T13:53:47Z

src/JuMPArray.jl

+struct JuMPArray{T,N,Ax} <: AbstractArray{T,N}
+    data::Array{T,N}
+    axes::Ax
+    lookup::Vector{Dict} # TODO: correctly type return type of the Dict as Int


Is this TODO out of scope for this PR?

joehuchette · 2017-09-19T13:58:45Z

src/JuMPArray.jl

+# AbstractArray interface
+
+Base.linearindices(A::JuMPArray) = error("JuMPArray does not support this operation.")
+Base.size(A::JuMPArray) = error("JuMPArray does not define this operation")


Seems like this might something users could reasonably expect to work

We don't define size for the old JuMPArray, and neither does OffsetArrays. Defining it causes end to return the wrong result.

joehuchette · 2017-09-19T14:07:19Z

src/containers.jl

+3. Otherwise, generate an empty `Dict{Any,T}`.
+
+The second element of the return tuple is a `Bool`, `true` if the container type
+automatically checks for duplicate terms in the index sets and `false` otherwise.


Might it be simpler to add a autocheckforduplicates(::Symbol) function, rather than explicitly returning a boolean?

The issue is that it depends on the processing of indexvars and indexsets (hasdependentsets). I agree that this solution isn't too tidy though.

mlubin · 2017-09-20T00:02:15Z

Do you have a sense of the performance relative to the current implementation?

There's no reason to benchmark JuMPArray right now because it will just be slow. When rewritten with generated functions it should have similar performance, except no special handling for interval sets (but this could be added transparently without breaking the API).

joehuchette · 2017-09-20T00:47:11Z

OK. What's the roadmap then with this PR, generated functions, and AxisArrays?

mlubin · 2017-09-20T01:00:29Z

What's the roadmap then with this PR, generated functions, and AxisArrays?

I'd like to merge this PR soon and get back to MOI work. When MOI is close to being wrapped up we can revisit and see if AxisArrays has implemented the functionality we need or if we need to tidy up JuMPArray.

mlubin force-pushed the ml/containers2 branch from 0607602 to 3dcd39b Compare September 16, 2017 03:03

jump-dev deleted a comment from codecov bot Sep 16, 2017

mlubin force-pushed the ml/containers2 branch from 3dcd39b to 497083b Compare September 17, 2017 20:23

mlubin mentioned this pull request Sep 17, 2017

UndefError in printing JuMPArray with index set of tuples #922

Closed

mlubin force-pushed the ml/containers2 branch 4 times, most recently from 1438546 to 041d4b7 Compare September 19, 2017 02:32

mlubin force-pushed the ml/containers2 branch from 041d4b7 to 852a3af Compare September 19, 2017 02:35

mlubin requested a review from joehuchette September 19, 2017 02:39

mlubin changed the title ~~WIP: Container revamp~~ Container revamp Sep 19, 2017

joehuchette approved these changes Sep 19, 2017

View reviewed changes

tidy type of lookup field

aa7a558

explain why we don't define size

8a49636

coverage improvement

095c030

mlubin merged commit 9466fe9 into jump/moi Sep 20, 2017

mlubin deleted the ml/containers2 branch September 20, 2017 11:29

mlubin mentioned this pull request Sep 20, 2017

initial transition to MOI #1081

Merged

blegat mentioned this pull request Nov 10, 2018

✨ Implement size for JuMPArray #1595

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container revamp #1102

Container revamp #1102

mlubin commented Sep 16, 2017 •

edited

Loading

codecov bot commented Sep 17, 2017

codecov bot commented Sep 17, 2017 •

edited

Loading

mlubin commented Sep 19, 2017 •

edited

Loading

joehuchette left a comment

joehuchette Sep 19, 2017

joehuchette Sep 19, 2017

mlubin Sep 19, 2017

joehuchette Sep 19, 2017

mlubin Sep 20, 2017

mlubin commented Sep 20, 2017

joehuchette commented Sep 20, 2017

mlubin commented Sep 20, 2017

Container revamp #1102

Container revamp #1102

Conversation

mlubin commented Sep 16, 2017 • edited Loading

codecov bot commented Sep 17, 2017

Codecov Report

codecov bot commented Sep 17, 2017 • edited Loading

Codecov Report

mlubin commented Sep 19, 2017 • edited Loading

joehuchette left a comment

Choose a reason for hiding this comment

joehuchette Sep 19, 2017

Choose a reason for hiding this comment

joehuchette Sep 19, 2017

Choose a reason for hiding this comment

mlubin Sep 19, 2017

Choose a reason for hiding this comment

joehuchette Sep 19, 2017

Choose a reason for hiding this comment

mlubin Sep 20, 2017

Choose a reason for hiding this comment

mlubin commented Sep 20, 2017

joehuchette commented Sep 20, 2017

mlubin commented Sep 20, 2017

mlubin commented Sep 16, 2017 •

edited

Loading

codecov bot commented Sep 17, 2017 •

edited

Loading

mlubin commented Sep 19, 2017 •

edited

Loading