WIP: rework TileIterator #27

johnnychen94 · 2020-12-01T22:14:22Z

It's a bit hard to extend the current TileIterator's behavior, so here I plan to rework it here. The code design basically follows the same idea of UnitRange-CartesianIndices.

Improvements:

support arbitrary stride:

julia> TiledIndices((1:15, 1:10), (5, 5), (2, 3))
6×3 TiledIndices{2, Int64, UnitRange{Int64}}:
 (1:6, 1:6)    (1:6, 4:9)    (1:6, 7:10)
 (3:8, 1:6)    (3:8, 4:9)    (3:8, 7:10)
 (5:10, 1:6)   (5:10, 4:9)   (5:10, 7:10)
 (7:12, 1:6)   (7:12, 4:9)   (7:12, 7:10)
 (9:14, 1:6)   (9:14, 4:9)   (9:14, 7:10)
 (11:15, 1:6)  (11:15, 4:9)  (11:15, 7:10)

support CartesianIndices:

julia> R = TiledIndices(CartesianIndices((15, 10)), (5, 5));
julia> R[1]
6×6 CartesianIndices{2, Tuple{UnitRange{Int64}, UnitRange{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)  CartesianIndex(1, 3)  …  CartesianIndex(1, 5)  CartesianIndex(1, 6)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)  CartesianIndex(2, 3)     CartesianIndex(2, 5)  CartesianIndex(2, 6)
 CartesianIndex(3, 1)  CartesianIndex(3, 2)  CartesianIndex(3, 3)     CartesianIndex(3, 5)  CartesianIndex(3, 6)
 CartesianIndex(4, 1)  CartesianIndex(4, 2)  CartesianIndex(4, 3)     CartesianIndex(4, 5)  CartesianIndex(4, 6)
 CartesianIndex(5, 1)  CartesianIndex(5, 2)  CartesianIndex(5, 3)     CartesianIndex(5, 5)  CartesianIndex(5, 6)
 CartesianIndex(6, 1)  CartesianIndex(6, 2)  CartesianIndex(6, 3)  …  CartesianIndex(6, 5)  CartesianIndex(6, 6)

TODO:

add RelaxStride tiling strategy
docs and tests
depreacate TileIterator

cc: @jw3126

closes #24
closes #25
closes #26

codecov · 2020-12-02T05:27:50Z

Codecov Report

Merging #27 (3c5977d) into master (539797c) will decrease coverage by 1.25%.
The diff coverage is 94.33%.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   98.16%   96.91%   -1.26%     
==========================================
  Files           2        5       +3     
  Lines         109      162      +53     
==========================================
+ Hits          107      157      +50     
- Misses          2        5       +3

Impacted Files	Coverage Δ
src/TiledIteration.jl	`96.22% <ø> (ø)`
src/compat.jl	`0.00% <0.00%> (ø)`
src/tileindices.jl	`100.00% <100.00%> (ø)`
src/tilerange.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 539797c...3c5977d. Read the comment docs.

jw3126 · 2020-12-03T09:15:53Z

I like that this exports TiledUnitRange and puts emphasis on the fact that all we do is a product of 1d tilings.
On the low level #23 already did this and was even a bit more flexible. But this PR does it maybe cleaner and exports TiledUnitRange.

So the plan would be to add more types of tiled unit ranges and a high level API for things like RelaxStride?.

jw3126 · 2020-12-03T09:16:32Z

src/tile.jl

+### TiledIndices
+
+struct TiledIndices{N, T, R} <: AbstractArray{R, N}
+    indices::NTuple{N, TiledUnitRange{T, R}}


Why restict to TiledUnitRange{T, R}? Why not any AbstractVector of ranges?

johnnychen94 · 2020-12-05T22:55:25Z

So the plan would be to add more types of tiled unit ranges and a high level API for things like RelaxStride?.

@jw3126 I think 52a8328 answers this.

I borrowed the same relationship between AbstractUnitRange and CartesianIndices, and use it to build up our tile-version
ranges and indices. Strategies like RelaxStride are implemented by AbstractTileStrategy, which is an adapter for AbstractTileRange and TileIndices. So we still have a flexible API to use:

julia> TileIndices((1:4, ), FixedTile((4, ))) == TileIterator((1:4, ), RelaxLastTile((4, )))
true
julia> TileIndices((1:4, ), (4, )) == TileIterator((1:4, ), (4, ))
true

RelaxLastTile is now fully covered by FixedTileRange with two more features: custom strides and keep_last.

julia> TileIndices((1:5,), 4, 2) == [(1:4,), (3:5,)]
true

julia> TileIndices((1:5,), 4) == [(1:4,), (5:5,)]
true

julia> TileIndices((1:5,), 4; keep_last=false) == [(1:4,)]
true

Performance is also the same. (marginally better, actually)

I haven't implemented RelaxStride right now, but the current design allows such an extension. We only need to define a new range type and a strategy type for this. I'll do this in the next step (still in this PR), and when that's done, I'll step to deprecate the old usage we introduced in #23.

The best, best thing I like about this PR is that it now supports all the indices semantics that https://docs.julialang.org/en/v1.5/manual/arrays/#man-supported-index-types describes: "An array of scalar indices".

julia> FixedTileRange(CartesianIndices((2:10,)), 4)
3-element FixedTileRange{CartesianIndices{1, Tuple{UnitRange{Int64}}}, Int64, CartesianIndices{1, Tuple{UnitRange{Int64}}}}:
 [CartesianIndex(2,), CartesianIndex(3,), CartesianIndex(4,), CartesianIndex(5,)]
 [CartesianIndex(6,), CartesianIndex(7,), CartesianIndex(8,), CartesianIndex(9,)]
 [CartesianIndex(10,)]

julia> FixedTileRange(collect(2:10), 4)
3-element FixedTileRange{Vector{Int64}, Int64, Vector{Int64}}:
 [2, 3, 4, 5]
 [6, 7, 8, 9]
 [10]

ping @timholy for a feedback

johnnychen94 · 2020-12-05T23:15:52Z

src/tilerange.jl

+```jldoctest; setup=:(using TiledIteration)
+julia> FixedTileRange(CartesianIndices((1:10, )), 4)
+3-element FixedTileRange{CartesianIndices{1,Tuple{UnitRange{Int64}}},Int64,CartesianIndices{1,Tuple{UnitRange{Int64}}}}:
+ [CartesianIndex(1,), CartesianIndex(2,), CartesianIndex(3,), CartesianIndex(4,)]
+ [CartesianIndex(5,), CartesianIndex(6,), CartesianIndex(7,), CartesianIndex(8,)]
+ [CartesianIndex(9,), CartesianIndex(10,)]
+```


It's even more ugly and unstable when it comes to N-d case.

julia> TileIndices(CartesianIndices((1:20, 1:20)), (7, 7)) 3×3 TileIndices{Tuple{CartesianIndices{1, Tuple{UnitRange{Int64}}}, CartesianIndices{1, Tuple{UnitRange{Int64}}}}, 2, FixedTileRange{CartesianIndices{1, Tuple{UnitRange{Int64}}}, Int64, CartesianIndices{1, Tuple{UnitRange{Int64}}}}}: CartesianIndex{2}[CartesianIndex(1, 1) CartesianIndex(1, 2) … CartesianIndex(1, 6) CartesianIndex(1, 7); CartesianIndex(2, 1) CartesianIndex(2, 2) … CartesianIndex(2, 6) CartesianIndex(2, 7); … ; CartesianIndex(6, 1) CartesianIndex(6, 2) … CartesianIndex(6, 6) CartesianIndex(6, 7); CartesianIndex(7, 1) CartesianIndex(7, 2) … CartesianIndex(7, 6) CartesianIndex(7, 7)] … CartesianIndex{2}[CartesianIndex(1, 15) CartesianIndex(1, 16) … CartesianIndex(1, 19) CartesianIndex(1, 20); CartesianIndex(2, 15) CartesianIndex(2, 16) … CartesianIndex(2, 19) CartesianIndex(2, 20); … ; CartesianIndex(6, 15) CartesianIndex(6, 16) … CartesianIndex(6, 19) CartesianIndex(6, 20); CartesianIndex(7, 15) CartesianIndex(7, 16) … CartesianIndex(7, 19) CartesianIndex(7, 20)] CartesianIndex{2}[CartesianIndex(8, 1) CartesianIndex(8, 2) … CartesianIndex(8, 6) CartesianIndex(8, 7); CartesianIndex(9, 1) CartesianIndex(9, 2) … CartesianIndex(9, 6) CartesianIndex(9, 7); … ; CartesianIndex(13, 1) CartesianIndex(13, 2) … CartesianIndex(13, 6) CartesianIndex(13, 7); CartesianIndex(14, 1) CartesianIndex(14, 2) … CartesianIndex(14, 6) CartesianIndex(14, 7)] CartesianIndex{2}[CartesianIndex(8, 15) CartesianIndex(8, 16) … CartesianIndex(8, 19) CartesianIndex(8, 20); CartesianIndex(9, 15) CartesianIndex(9, 16) … CartesianIndex(9, 19) CartesianIndex(9, 20); … ; CartesianIndex(13, 15) CartesianIndex(13, 16) … CartesianIndex(13, 19) CartesianIndex(13, 20); CartesianIndex(14, 15) CartesianIndex(14, 16) … CartesianIndex(14, 19) CartesianIndex(14, 20)] CartesianIndex{2}[CartesianIndex(15, 1) CartesianIndex(15, 2) … CartesianIndex(15, 6) CartesianIndex(15, 7); CartesianIndex(16, 1) CartesianIndex(16, 2) … CartesianIndex(16, 6) CartesianIndex(16, 7); … ; CartesianIndex(19, 1) CartesianIndex(19, 2) … CartesianIndex(19, 6) CartesianIndex(19, 7); CartesianIndex(20, 1) CartesianIndex(20, 2) … CartesianIndex(20, 6) CartesianIndex(20, 7)] CartesianIndex{2}[CartesianIndex(15, 15) CartesianIndex(15, 16) … CartesianIndex(15, 19) CartesianIndex(15, 20); CartesianIndex(16, 15) CartesianIndex(16, 16) … CartesianIndex(16, 19) CartesianIndex(16, 20); … ; CartesianIndex(19, 15) CartesianIndex(19, 16) … CartesianIndex(19, 19) CartesianIndex(19, 20); CartesianIndex(20, 15) CartesianIndex(20, 16) … CartesianIndex(20, 19) CartesianIndex(20, 20)]

and it simply crashes during the display if I use CartesianIndices((1:512, 1:512))

Is there a way to display those CartesianIndices as

[CartesianIndex(1, 1):CartesianIndex(1, 7), CartesianIndex(1, 1):CartesianIndex(8, 15), CartesianIndex(1, 1):CartesianIndex(16, 20)]

while still keep the current show behavior?

I vaguely remember that we have a top_level keyword for display related function, but can't find the name of it now.

johnnychen94 · 2020-12-05T23:28:27Z

Tests pass for Julia>=1.1, efforts will be made to keep 1.0 compatibility (although I don't use it...)

* oneunit for CartesianIndex * (:) method for CartesianIndex * IdentityUnitRange

johnnychen94 · 2020-12-06T21:22:12Z

Question: for RelaxStride with StepRange, what's the "correct" result?

Case 1:

julia> r = RoundedTileRange(1:2:9, 3)
3-element RoundedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:5
 3:2:7
 5:2:9

julia> r = RoundedTileRange(1:2:9, 3)
3-element RoundedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:5
 3:2:7
 4:2:8

julia> r = RoundedTileRange(1:2:9, 3)
2-element RoundedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:5
 5:2:9

# FixedTileRange
julia> r = FixedTileRange(1:2:9, 3)
2-element FixedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:5
 4:2:8

Case 2:

julia> r = RoundedTileRange(1:2:9, 4)
3-element RoundedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:7
 2:2:8
 3:2:9

julia> r = RoundedTileRange(1:2:9, 4)
2-element RoundedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:7
 3:2:9

# FixedTileRange
julia> r = FixedTileRange(1:2:9, 4)
2-element FixedTileRange{StepRange{Int64, Int64}, Int64, StepRange{Int64, Int64}}:
 1:2:7
 5:2:9

Seems to be the last one (as it has the same length to the FixedTileRange case)?

jw3126 · 2020-12-06T21:57:01Z

I also favor the last one. Alternatively we could raise an error, in case of a non unit range.

johnnychen94 · 2020-12-07T08:56:25Z

Alternatively we could raise an error, in case of a non unit range.

Possibly, but I'd like to give a solid support for all the valid indices type that Base defines. I could mark StepRange as an experimental support and doc that result might change in the future.

jw3126 · 2020-12-07T10:27:40Z

I could mark StepRange as an experimental support and doc that result might change in the future.

Sounds good!

timholy

This mostly looks good. I didn't investigate the "crash" but I wonder if it comes from type parameter issues?

timholy · 2020-12-10T12:30:49Z

src/tilerange.jl

+    `FixedTileRange(collect(1:10), 4)` creates a new `Vector` of length `4` everytime when
+    `getindex` is called.
+"""
+struct FixedTileRange{R, T, RP} <: AbstractTileRange{R}


The parameters could use some clarificiation, maybe just by subtyping, e.g., R<:AbstractVector{<:Integer} or something?

src/tilerange.jl

timholy · 2020-12-10T12:36:00Z

src/tilerange.jl

+
+_eltype(::Type{R}) where R<:OrdinalRange = StepRange{eltype(R), eltype(R)}
+_eltype(::Type{R}) where R<:AbstractUnitRange = UnitRange{eltype(R)}
+_eltype(::Type{R}) where R<:AbstractVector = R # this includes CartesianIndices{1}


You're standardizing OrdinalRange and AbstractUnitRange but allowing any other type of AbstractVector? Does Vector{Int} count?

Yes, Vector{Int} counts even though it is not memory-efficient. I choose to support that because Vector{Int} is a valid indices type.

Supporting this enables us to pass lazy evaluated vectors, even though I'm not sure if there's practical usage right now. Currently, I'm mostly concerned about CartesianIndices and step range.

timholy · 2020-12-10T12:42:18Z

src/tilerange.jl

+    _fixedtile_length(r.indices[1], n, Δ, keep_last)
+end
+function _fixedtile_length(r::AbstractVector{T}, n, Δ, keep_last) where T<:Integer
+    _fixedtile_length(UnitRange{T}(first(r), last(r)), n, Δ, keep_last)


Maybe convert(UnitRange{T}, r) just to make sure this is consistent with what is being passed in? Or are you intending to support r = 1:3:10?

Yes, StepRange and Vector{Int} are designed to be supported.

julia> FixedTileRange(1:2:10, 2) 4-element FixedTileRange{StepRange{Int64,Int64},Int64,StepRange{Int64,Int64}}: 1:2:3 3:2:5 5:2:7 7:2:9 julia> FixedTileRange([2, 3, 4, 5, 6], 2) 3-element FixedTileRange{Array{Int64,1},Int64,Array{Int64,1}}: [2, 3] [4, 5] [6]

I still need to figure out how this should be done for arbitrary vectors and step ranges; they're not working perfectly right now.

julia> FixedTileRange(collect(1:2:10), 2) # this is wrong 5-element FixedTileRange{Array{Int64,1},Int64,Array{Int64,1}}: [1, 2] [3, 4] [5, 6] [7, 8] [9]

timholy · 2020-12-10T12:54:18Z

src/tilerange.jl

+        _length = _fixedtile_length(parent, n, Δ, keep_last)
+        new{_eltype(R), T, R}(parent, n, Δ, keep_last, _length)
+    end
+end


It's interesting that this is conceptually equivalent to StepRange(1:5, BroadcastInt(2), 7:11) (a range-of-ranges). Should we consider the FixedTileRange in terms of the range API?

Hmmm, not sure if I understand it right, do you mean StepRange(1:5, BroadcastInt(2), 7:11) is equivalent to

julia> TileIndices((1:5, 7:11), 2) 3×3 TileIndices{Tuple{UnitRange{Int64},UnitRange{Int64}},2,FixedTileRange{UnitRange{Int64},Int64,UnitRange{Int64}}}: (1:2, 7:8) (1:2, 9:10) (1:2, 11:11) (3:4, 7:8) (3:4, 9:10) (3:4, 11:11) (5:5, 7:8) (5:5, 9:10) (5:5, 11:11)

? I couldn't find what BroadcastInt is.

Should we consider the FixedTileRange in terms of the range API?

I would be largely relieved if we could separate the indices and tile indices world, just like how we are doing with normal array and fancy block arrays. If that's what you were thinking about.

or are you saying to could make something like FixedTileRange(start, stop; length, Δ, n) only to mimic range's usage? That would be a little bit hard because I do plan to support all indices types, including the vectors and CartesianIndices. Mimicking the range API won't improve the useability much IMO.

timholy · 2020-12-10T13:18:18Z

src/tilerange.jl

+end
+
+Base.size(r::FixedTileRange) = (r.length, )
+Base.length(r::FixedTileRange) = r.length


I don't think you need to define this (the fallbacks do it).

johnnychen94 · 2021-01-25T08:02:54Z

StepRange and Vector support has become mind-puzzling and controversial. For this reason, I plan to first support UnitRange only in this PR (removing all the codes related to StepRange and Vector), and adding StepRange and Vector back in following PR(s), if necessary.

johnnychen94 added 2 commits December 2, 2020 06:09

WIP: rework TileIterator

a2d3b5b

add CartesianIndices support

42aca10

johnnychen94 changed the title ~~WIP: allow arbitrary tile stride (rework TileIterator)~~ WIP: rework TileIterator Dec 1, 2020

johnnychen94 mentioned this pull request Dec 1, 2020

support CartesianIndices as axes for TileIterator #26

Open

jw3126 reviewed Dec 3, 2020

View reviewed changes

johnnychen94 added 2 commits December 6, 2020 06:26

implement FixedTileRange, FixedTile and TileIndices

52a8328

fix doctest

9946e31

johnnychen94 commented Dec 5, 2020

View reviewed changes

johnnychen94 added 3 commits December 6, 2020 20:19

compat fix for Julia 1.0

855918d

* oneunit for CartesianIndex * (:) method for CartesianIndex * IdentityUnitRange

ignore Manifest.toml

519a984

add StepRange support

3c5977d

johnnychen94 mentioned this pull request Dec 9, 2020

CompatHelper: bump compat for "TiledIteration" to "0.3" JuliaImages/ImageMorphology.jl#37

Merged

timholy reviewed Dec 10, 2020

View reviewed changes

johnnychen94 mentioned this pull request Dec 29, 2020

[WIP] rework SuperPixel and SuperPixelImage with Array interface johnnychen94/SuperPixels.jl#6

Open

johnnychen94 mentioned this pull request Aug 22, 2021

Construct ranges of CartesianIndexes JuliaLang/julia#41960

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: rework TileIterator #27

WIP: rework TileIterator #27

johnnychen94 commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

jw3126 commented Dec 3, 2020 •

edited

Loading

jw3126 Dec 3, 2020

johnnychen94 commented Dec 5, 2020 •

edited

Loading

johnnychen94 Dec 5, 2020 •

edited

Loading

johnnychen94 Dec 10, 2020

johnnychen94 commented Dec 5, 2020

johnnychen94 commented Dec 6, 2020 •

edited

Loading

jw3126 commented Dec 6, 2020

johnnychen94 commented Dec 7, 2020

jw3126 commented Dec 7, 2020

timholy left a comment

timholy Dec 10, 2020

timholy Dec 10, 2020

johnnychen94 Dec 10, 2020

timholy Dec 10, 2020

johnnychen94 Dec 10, 2020

timholy Dec 10, 2020

johnnychen94 Dec 10, 2020

johnnychen94 Dec 10, 2020

timholy Dec 10, 2020

johnnychen94 commented Jan 25, 2021 •

edited

Loading

WIP: rework TileIterator #27

Are you sure you want to change the base?

WIP: rework TileIterator #27

Conversation

johnnychen94 commented Dec 1, 2020 • edited Loading

codecov bot commented Dec 2, 2020 • edited Loading

Codecov Report

jw3126 commented Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

johnnychen94 commented Dec 5, 2020 • edited Loading

johnnychen94 Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnychen94 commented Dec 5, 2020

johnnychen94 commented Dec 6, 2020 • edited Loading

jw3126 commented Dec 6, 2020

johnnychen94 commented Dec 7, 2020

jw3126 commented Dec 7, 2020

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnychen94 commented Jan 25, 2021 • edited Loading

johnnychen94 commented Dec 1, 2020 •

edited

Loading

codecov bot commented Dec 2, 2020 •

edited

Loading

jw3126 commented Dec 3, 2020 •

edited

Loading

johnnychen94 commented Dec 5, 2020 •

edited

Loading

johnnychen94 Dec 5, 2020 •

edited

Loading

johnnychen94 commented Dec 6, 2020 •

edited

Loading

johnnychen94 commented Jan 25, 2021 •

edited

Loading