Fix map and lastwithindex bugs in ResizableArray
#228
Conversation
| @@ -241,7 +244,7 @@ end | |||
| function lastwithindex(array::ResizableArray{T, V, N}) where {T, V, N} | |||
| for index in reverse(CartesianIndices(reverse(size(array)))) #TODO improve performance of this function since it uses splatting | |||
There was a problem hiding this comment.
I think the TODO comment is old, the splatting by itself does not hurt performance, but wrong usage of splatting does. Here everything should be fine imo
src/resizable_array.jl
Outdated
| @@ -181,14 +181,17 @@ function Base.iterate(array::ResizableArray, state) | |||
| return nothing | |||
| end | |||
| nindex, nstate = niterate | |||
| while !isassigned(array, nindex.I...) | |||
There was a problem hiding this comment.
This might be dangerous, lets discuss tomorrow ?, I didn't do this operation for a reason. Otherwise the zip with a Matrix is error prone. I wanted it to throw an error if something is not assigned. Maybe we should make a special method for map? Or we could add an option to the iterate method to skip things that aren't set, and then map could use that.
There was a problem hiding this comment.
E.g. this branch:
julia> s = GraphPPL.ResizableArray(Float64, Val(2))
0×0 GraphPPL.ResizableArray{Float64, Vector{Vector{Float64}}, 2}
julia> s[1, 1] = 1.0;
julia> s[2, 2] = 2.0;
julia> s[3, 3] = 3.0;
julia> map(sum, zip(s, Diagonal(ones(3))))
3×3 Matrix{Float64}:
2.0 2.0 0.0
2.62258e-314 1.0 0.0
2.62258e-314 3.0 2.2241e-314On the main it throws (which is better than silently getting wrong result):
julia> map(sum, zip(s, Diagonal(ones(3))))
ERROR: BoundsError: attempt to access 1-element Vector{Float64} at index [2]The zip operation is used in ReactiveMP to connect ResizableArray with some data. So a user might give Diagonal data and get some wrong results instead of an error
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #228 +/- ##
==========================================
+ Coverage 90.90% 91.07% +0.17%
==========================================
Files 15 15
Lines 1979 1995 +16
==========================================
+ Hits 1799 1817 +18
+ Misses 180 178 -2 ☔ View full report in Codecov by Sentry. |
No description provided.