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 7 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
12 changes: 12 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,25 @@ end
end

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

immutable JuMPKey{T<:Tuple}
jk::T
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
89 changes: 82 additions & 7 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,86 @@ 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.


immutable IndexableIndexsets end
immutable NotIndexableIndexsets end
Copy link
Contributor

@joehuchette joehuchette Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of using types here, a simple true/false should suffice. Using method_exists below means indexability will lead to dynamic function calls anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that traits are cleaner for dispatch. I agree that now it isn't impacting performance because the dispatch of the function next is done using the type of k that is either a Tuple for the new iterator or an Integer for the old one. But if we manage to change the new iterator to use an Integer state as well, traits will be needed. I don't mind deleting them right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I would prefer booleans, as this code is already a bit difficult for me to parse. We can always add complexity later on as-needed


function indexability(x::JuMPArray)
allsetsindexable = true
for i in 1:length(x.indexsets)
if !method_exists(getindex, (typeof(x.indexsets[i]),))
allsetsindexable = false
break
end
end

if allsetsindexable
IndexableIndexsets()
else
NotIndexableIndexsets()
end
end

Base.start(it::KeyIterator) = _start(it, indexability(it.x))
_start(it::KeyIterator, ::IndexableIndexsets) = start(it.x.innerArray)
_start(it::KeyIterator, ::NotIndexableIndexsets) = notindexable_start(it.x)

@generated function notindexable_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) =

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

function Base.next(it::KeyIterator, k::Tuple)
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,1) #The second 1 is needed to ambiguity with ints
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain this further please?

Copy link
Contributor Author

@IssamT IssamT Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The first element of the tuple is used for indicating that we are done. The second 1 here is added because otherwise the dispatch of the function n̶e̶x̶t̶ done fails and the function n̶e̶x̶t̶ done with Integer state is called instead of the one with Tuple state

Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this mess with the type stability of the function? Could there a cleaner way to implement this without using a sentinel value return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You re right. It s not very clean. The following modification doesn't change the performance (I tested it with several containers as index sets) but I will still do it to keep the exact return type everywhere

-        return cartesian_key, (1,1) #The second 1 is needed to ambiguity with ints
+        it.next_k_cache[1] = 1
+        return cartesian_key, tuple(it.next_k_cache...)

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
cartesian_key, tuple(it.next_k_cache...)
end

function Base.done(it::KeyIterator, k::Tuple)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be

Base.done(it::KeyIterator, k::Tuple) = (k[1] == 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will fix that as well on my next commit.

return k[1] == 1
end

@generated __next{T,N,NT}(x::JuMPArray{T,N,NT}, k::Integer) =
quote
subidx = ind2sub(size(x),k)
$(Expr(:tuple, [:(x.indexsets[$i][subidx[$i]]) for i in 1:N]...)), next(x.innerArray,k)[2]
JuMPKey($(Expr(:tuple, [:(x.indexsets[$i][subidx[$i]]) for i in 1:N]...))), next(x.innerArray,k)[2]
end
Base.next(it::KeyIterator, k) = __next(it.x,k)
Base.done(it::KeyIterator, k) = done(it.x.innerArray, k)
Base.next(it::KeyIterator, k) = __next(it.x,k::Integer)
Base.done(it::KeyIterator, k) = done(it.x.innerArray, k::Integer)

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