Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

Merged
merged 8 commits into from
Sep 27, 2016
Merged

Fixing KeyIterator used for JuMPArray to work also when index sets are non indexable. #836

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
734b58e
keys() work as expected now for JuMPArray when index set is not index…
IssamT Aug 21, 2016
4204781
minor comment modification
IssamT Aug 21, 2016
5e59724
fixed indentation to be 4 spaces
IssamT Aug 23, 2016
452743c
moved the initialization of KeyIterator state from constructor to Bas…
IssamT Aug 23, 2016
cc53bdc
improved KeyIterator of JuMPArrays in two ways:
IssamT Sep 3, 2016
ab0d709
added JuMPKey, a tuple wrapper, to be used by keys(::JuMPArray)
IssamT Sep 8, 2016
ee819c7
added specialization of the iterator to keep good performance
IssamT Sep 12, 2016
c980b33
- removed JuMPKey (to be added in separate PR)
IssamT Sep 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/JuMPArray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,26 @@ end
end

Base.getindex(d::JuMPArray, ::Colon) = d.innerArray[:]

immutable JuMPKey
jk::Tuple
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does performance improve if this is instead

immutable JuMPKey{T<:Tuple}
    jk::T
end

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't @joehuchette . The wrapper is the second modification I have made and it is the first one that had a negative impact on the performance, namely using iterators instead of indexes. Maybe I confused you, by not opening another PR. I thought it is related ...

Should I open a new PR for the JuMPKey wrapper?


  0.143376 seconds (110.83 k allocations: 2.241 MB)
  0.009154 seconds (60.00 k allocations: 937.559 KB)
  0.009054 seconds (60.00 k allocations: 937.559 KB)
  0.008394 seconds (60.00 k allocations: 937.559 KB)
  0.008086 seconds (60.00 k allocations: 937.559 KB)
  0.009080 seconds (60.00 k allocations: 937.559 KB)
  0.008189 seconds (60.00 k allocations: 937.559 KB)
  0.008068 seconds (60.00 k allocations: 937.559 KB)
  0.008026 seconds (60.00 k allocations: 937.559 KB)
  0.009072 seconds (60.00 k allocations: 937.559 KB)
  0.008065 seconds (60.00 k allocations: 937.559 KB)
  0.008093 seconds (60.00 k allocations: 937.559 KB)
  0.009081 seconds (60.00 k allocations: 937.559 KB)
  0.021784 seconds (60.00 k allocations: 937.559 KB)
  0.016168 seconds (60.00 k allocations: 937.559 KB)
  0.014673 seconds (60.00 k allocations: 937.559 KB)
  0.012351 seconds (60.00 k allocations: 937.559 KB)
  0.011812 seconds (60.00 k allocations: 937.559 KB)
  0.010432 seconds (60.00 k allocations: 937.559 KB)
  0.010156 seconds (60.00 k allocations: 937.559 KB)

end

Base.start(x::JuMPKey) = Base.start(x.jk)
Base.next(x::JuMPKey, i) = Base.next(x.jk, i)
Base.done(x::JuMPKey, i) = Base.done(x.jk, i)
Base.length(x::JuMPKey) = Base.length(x.jk)

@generated function Base.getindex{T,N,NT<:NTuple}(d::JuMPArray{T,N,NT}, idx...)
if N != length(idx)
error("Indexed into a JuMPArray with $(length(idx)) indices (expected $N indices)")
end
Expr(:call, :getindex, :(d.innerArray), _to_cartesian(d,NT,idx)...)
end

Base.getindex(d::JuMPArray, x::JuMPKey) = Base.getindex(d, x.jk...)


@generated function Base.setindex!{T,N,NT<:NTuple}(d::JuMPArray{T,N,NT}, v::T, idx...)
if N != length(idx)
error("Indexed into a JuMPArray with $(length(idx)) indices (expected $N indices)")
Expand Down
62 changes: 54 additions & 8 deletions src/JuMPContainer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,10 @@ Base.ndims{T,N}(x::JuMPDict{T,N}) = N
Base.abs(x::JuMPDict) = map(abs, x)
# avoid dangerous behavior with "end" (#730)
Base.endof(x::JuMPArray) = error("endof() (and \"end\" syntax) not implemented for JuMPArray objects.")
Base.size(x::JuMPArray) = error("size (and \"end\" syntax) not implemented for JuMPArray objects. Use JuMP.size if you want to access the dimensions.")
Base.size(x::JuMPArray,k) = error("size (and \"end\" syntax) not implemented for JuMPArray objects. Use JuMP.size if you want to access the dimensions.")
Base.size(x::JuMPArray) = error(string("size (and \"end\" syntax) not implemented for JuMPArray objects.",
"Use JuMP.size if you want to access the dimensions."))
Base.size(x::JuMPArray,k) = error(string("size (and \"end\" syntax) not implemented for JuMPArray objects.",
" Use JuMP.size if you want to access the dimensions."))
size(x::JuMPArray) = size(x.innerArray)
size(x::JuMPArray,k) = size(x.innerArray,k)
# for uses of size() within JuMP
Expand Down Expand Up @@ -183,13 +185,57 @@ Base.length(it::ValueIterator) = length(it.x)

type KeyIterator{JA<:JuMPArray}
x::JA
dim::Int
next_k_cache::Array{Any,1}
function KeyIterator(d)
n = ndims(d.innerArray)
new(d, n, Array(Any, n+1))
end
end

KeyIterator{JA}(d::JA) = KeyIterator{JA}(d)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method necessary with the inner constructor above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleting line 196 gives this error

ERROR: LoadError: MethodError: Cannot convert an object of type JuMP.JuMPArray{Float64,2,Tuple{UnitRange{Int32},UnitRange{Int32}}} to an object of type JuMP.KeyIterator{JA<:JuMP.JuMPArray}
This may have arisen from a call to the constructor JuMP.KeyIterator{JA<:JuMP.JuMPArray}(...),

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, strange. OK, thanks.


@generated function _start{T,N,NT}(x::JuMPArray{T,N,NT})
quote
$(Expr(:tuple, 0, [:(start(x.indexsets[$i])) for i in 1:N]...))
end
end
Base.start(it::KeyIterator) = start(it.x.innerArray)
@generated __next{T,N,NT}(x::JuMPArray{T,N,NT}, k) =

Base.start(it::KeyIterator) = _start(it.x)

@generated function _next{T,N,NT}(x::JuMPArray{T,N,NT}, k)
quote
subidx = ind2sub(size(x),k)
$(Expr(:tuple, [:(x.indexsets[$i][subidx[$i]]) for i in 1:N]...)), next(x.innerArray,k)[2]
$(Expr(:tuple, [:(next(x.indexsets[$i], k[$i+1])[1]) for i in 1:N]...))
end
end

function Base.next(it::KeyIterator, k)
cartesian_key = JuMPKey(_next(it.x, k))
pos = -1
for i in 1:it.dim
if(!done(it.x.indexsets[i], next(it.x.indexsets[i], k[i+1])[2] ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style point: it's preferred to use whitespace, not parentheses, in if statements:

if !done(it.x.indexsets[i], next(it.x.indexsets[i], k[i+1])[2])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. I will fix it in my next commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, just a minor point

pos = i
break
end
end
if pos == - 1
return cartesian_key, (1)
end
it.next_k_cache[1] = 0
for i in 1:it.dim
if i < pos
it.next_k_cache[i+1] = start(it.x.indexsets[i])
elseif i == pos
it.next_k_cache[i+1] = next(it.x.indexsets[i], k[i+1])[2]
else
it.next_k_cache[i+1] = k[i+1]
end
end
Base.next(it::KeyIterator, k) = __next(it.x,k)
Base.done(it::KeyIterator, k) = done(it.x.innerArray, k)
cartesian_key, tuple(it.next_k_cache...)
end

function Base.done(it::KeyIterator, k)
return k[1] == 1
end

Base.length(it::KeyIterator) = length(it.x.innerArray)