Support overlapping tiles #23

jw3126 · 2020-10-13T21:32:00Z

Close #22
This needs at least docs and more tests. But before I go into that, @johnnychen94 what do you think about the API/design of this PR?

codecov · 2020-10-13T21:54:03Z

Codecov Report

Merging #23 into master will decrease coverage by 2.89%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   96.00%   93.10%   -2.90%     
==========================================
  Files           1        2       +1     
  Lines          75      116      +41     
==========================================
+ Hits           72      108      +36     
- Misses          3        8       +5

Impacted Files	Coverage Δ
src/TiledIteration.jl	`96.22% <ø> (+0.22%)`	⬆️
src/tileiterator.jl	`90.47% <90.47%> (ø)`

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 8dcfb86...af9bd17. Read the comment docs.

johnnychen94 · 2020-10-14T06:37:19Z

As a prototype, there's no doubt that this version works for your purpose. I have thought about the enhancement of this package but haven't dig into all the possibilities and implementation details. Here are some of my thoughts; they are not what we must do, but I'd like to see things head approximately in that direction.

Codes in the current PR version seems a bit coupled together and might be very hard to extend. If it is possible, I feel it would be better and cleaner to implement a new Range type for the Balanced version, and reuse StepRange for the Fixed version. And keep the previous struct type as possible as we can.

struct TileIterator{N,C} <: AbstractArray{NTuple{N, UnitRange{Int}}, N}
    covers1d::C
end

IIUC covers1d here is a tuple of vectors, which isn't great in performance. We should make this vector lazily generated. I think that's why the previous struct type is designed that way.

Generally, I don't think it's a good direction to make it an array type. For simple cases like this, we could have efficient getindex implementation, but that's not always possible for other cases. Make it an iterator would be more efficient and extensible, but yes, it requires some effort to implement it.

I'm not sure if we should add stride keyword here, I feel it'd be better to

TileIterator(axes::Indices, ::IterationStrategy)

size and stride are just meta info to iteration strategy. I prefer to let TileIterator to normalize the axes input/output and let IterationStrategy to handle the dirty and complex iteration implementation.

Does every strategy has a stride meta? I don't know, I don't feel it good to assume that. We assume iteration along the column direction here, it might be good to assume that we also want to support iteration along row direction in the future. How would we solve that, adding a new columnmajor=true keyword? Probably, but that would exponentially increase the complexity.

In general, I want to get to a design where iteration strategies are pluggable and composable.

jw3126 · 2020-10-14T16:11:10Z

Thanks a lot for your comments. I agree with most of what you said, but have a different view on some points:

Codes in the current PR version seems a bit coupled together and might be very hard to extend. If it is possible, I feel it would be better and cleaner to implement a new Range type for the Balanced version, and reuse StepRange for the Fixed version. And keep the previous struct type as possible as we can.
struct TileIterator{N,C} <: AbstractArray{NTuple{N, UnitRange{Int}}, N}
    covers1d::C
end
IIUC covers1d here is a tuple of vectors, which isn't great in performance. We should make this vector lazily generated. I think that's why the previous struct type is designed that way.

There seems to be a misunderstanding covers1d is a tuple of AbstactVector{UnitRange{Int}}. It can be a tuple of Vector, but that is not the default. The default is lazy. Either StepRange or QuantizedRange in the balanced case. I think this is very flexible and easy to extend. I did not look into performance yet.
I could imagine, that a second field is needed for faster iteration, but I would like to avoid that if possible.

struct TileIterator{...}
covers1d::C
_fast_iteration_implementation_detail::F
end

Generally, I don't think it's a good direction to make it an array type. For simple cases like this, we could have efficient getindex implementation, but that's not always possible for other cases. Make it an iterator would be more efficient and extensible, but yes, it requires some effort to implement it.

I think in the above situation it is very natural to make an AbstractArray and getindex can be fast. And I think it will be easy for any tiling that is a product of 1d cases. If we have a case that is not a product, it should use another type. Something like

struct ExoticTileIterator <: NotAnAbstractArray
...
end

Maybe we should use tileiterator(axes, strategy) and it will create a ProductTiling <: AbstractArray or MyExoticTiling <: NotAnArray.

I'm not sure if we should add stride keyword here, I feel it'd be better to
TileIterator(axes::Indices, ::IterationStrategy)
size and stride are just meta info to iteration strategy. I prefer to let TileIterator to normalize the axes input/output and let IterationStrategy to handle the dirty and complex iteration implementation.

Excellent point I agree.

Does every strategy has a stride meta? I don't know, I don't feel it good to assume that. We assume iteration along the column direction here, it might be good to assume that we also want to support iteration along row direction in the future. How would we solve that, adding a new columnmajor=true keyword? Probably, but that would exponentially increase the complexity.

I fully agree here, is better to go with your ::IterationStrategy suggestion.

In general, I want to get to a design where iteration strategies are pluggable and composable.

Again I fully agree. I am however not sure, what pluggable and composable mean here concretely.

johnnychen94 · 2020-10-15T08:39:44Z

I'm a little bit with my own work here and I'm sorry that I can't give any detailed feedback on the code at this moment. It looks like you clearly know what should be done. My major concern about this PR and the changes is the performance overhead and extensibility. If that's done and I'm good with that.

IIUC covers1d here is a tuple of vectors, which isn't great in performance. We should make this vector lazily generated. I think that's why the previous struct type is designed that way.

There seems to be a misunderstanding covers1d is a tuple of AbstactVector{UnitRange{Int}}. It can be a tuple of Vector, but that is not the default. The default is lazy. Either StepRange or QuantizedRange in the balanced case. I think this is very flexible and easy to extend. I did not look into performance yet.

Hmmm, I don't quite get it here because the constructor tells me this.

function TileIterator(covers1d::NTuple{N, AbstractVector{UnitRange{Int}}}) where {N}
    C = typeof(covers1d)
    return TileIterator{N, C}(covers1d)
end

This representation might bring some overhead and I'd like to avoid that overhead. I probably am wrong here but still, a performance benchmark is needed to support this change.

Maybe we should use tileiterator(axes, strategy) and it will create a ProductTiling <: AbstractArray or MyExoticTiling <: NotAnArray.

Yeah, perhaps. It seems like this doesn't need to be included in this PR.

In general, I want to get to a design where iteration strategies are pluggable and composable.
Again I fully agree. I am however not sure, what pluggable and composable mean here concretely.

By composable I mean two iteration strategy can be composed together into another strategy. For example, Fixed() ∘ RowMajor() or something like this. Again, this doesn't need to be included in this PR but it would be better that we consider this possibility and extensibility.

jw3126 · 2020-10-15T10:55:33Z

Hmmm, I don't quite get it here because the constructor tells me this.
function TileIterator(covers1d::NTuple{N, AbstractVector{UnitRange{Int}}}) where {N}
    C = typeof(covers1d)
    return TileIterator{N, C}(covers1d)
end
This representation might bring some overhead and I'd like to avoid that overhead. I probably am wrong here but still, a performance benchmark is needed to support this change.

This constructor is a bit low level. If you ask by whatever API for the balanced case, you would get

typeof(covers1d) = Tuple{CoveredRange{QuantizedRange....

which is a bitstype and completly lazy. Did you worry because you thought e.g. this would allocate a Vector or are you worrying that the compiler does not properly inline away all the abstraction?

johnnychen94 · 2020-10-15T12:18:45Z

Ha, I get your point here; didn't realize that CoveredRange <: AbstractVector. I was worried about the additional overhead during getindex(::Vector, i) but since this is lazily generated, it looks good.

I was worried about the overhead during iteration, for example:

julia> titr = TileIterator((1:7, 3:6), tilesize=(2, 2), stride=Balanced((3, 2)));

julia> @btime getindex(titr, 2, 2)
  13.920 ns (0 allocations: 0 bytes)
(3:4, 5:6)

julia> titr = TileIterator((1:7, 3:6), tilesize=(2, 2), stride=Fixed((3, 2)));

julia> @btime getindex(titr, 2, 2)
  0.045 ns (0 allocations: 0 bytes)
(4:5, 5:6)

great that this has zero allocation. So yeah, this implementation is quite efficient already. Perhaps there's still some room to get getindex for Balanced faster; I'm a bit curious why it's slower here.

The time spent on construction is less important here since we generally don't need to repeatedly create TileIterators; it's still worth improving if that's not too hard.

This looks good to me now, a really nice play with the types and dispatches. I guess we just need to leave some room for future extensibility as we discussed. Then add docs and tests and I feel it could be good to merge.

jw3126 · 2020-10-15T20:48:04Z

src/tileiterator.jl

+
+# strategies
+export RelaxStride
+struct RelaxStride{N}


I thought, lets be conservative for now. I just added these two strategies. They are enough to cover the funcionality on master as well as my balanced tile size use case.

A different PR could implement a strategy that sets the stride to (1,1,1...) giving the behavior sketched here:
JuliaImages/ImageFiltering.jl#155 (comment)

For more advanced strides, we might want to do
tileiterator(axes, UnitStride())[begin:2:end, :, begin+2:4:end-2]

This works but might not be the most efficient way because indexing with [begin:2:end, :, begin+2:4:end-2] allocates a new array.

No need to implement in this PR, though. It is also very likely that reducing memory allocation only gives a marginal performance boost.

Yes you are right this allocates currently, we can fix it, when it becomes a practical obstacle.

src/tileiterator.jl

johnnychen94 · 2020-10-20T07:57:17Z

Probably we could just switch to use Travis for the Windows platform and sunset appveyor so that we don't get troubled with 32bit doctest.

jw3126 · 2020-10-20T08:24:37Z

I did remove appveyor.yml and travis passes on windows. Probably need to deactivate appveyor on the appveyor website or something?

johnnychen94

The current version looks good to me; probably the last round review. More docs and tests might be helpful, though.

src/tileiterator.jl

test/runtests.jl

src/tileiterator.jl

johnnychen94 · 2020-10-20T08:35:55Z

I did remove appveyor.yml and travis passes on windows. Probably need to deactivate appveyor on the appveyor website or something?

Will remove that once this PR is merged.

johnnychen94

LGTM, I think it's good to merge ~~once the test passes~~. This is not a breaking change, but I'll bump to v0.3.0 since it drops Julia v0.7 support.

ping @timholy in case he has some thoughts about this PR.

jw3126 · 2020-10-20T11:55:05Z

Thanks @johnnychen94 for the detailed review and feedback.

timholy · 2020-10-20T12:19:03Z

I've got this on my calendar to review tomorrow, sorry for the delay.

timholy

This is really great! In the future we may also want to move some of the tile-padding logic from ImageFiltering here, and this will be a good foundation.

src/TiledIteration.jl

src/tileiterator.jl

test/runtests.jl

Co-authored-by: Tim Holy <[email protected]>

timholy · 2020-10-22T10:48:23Z

There may be something weird going on. E.g., 75b2a90 doesn't match the commit-summary.

jw3126 · 2020-10-22T10:55:21Z

Yeah I messed something up. I think we can just squash this PR or should I try to clean the history?

timholy · 2020-10-22T11:01:19Z

I'll squash, no worries about that.

timholy · 2020-10-22T13:48:57Z

Thanks again, terrific to have this!

timholy · 2020-10-22T13:50:15Z

There's nothing breaking about this, right?

Seems a little weird to release such a major improvement as 0.2.6, but OTOH not having to do version bumps in dependencies has advantages.

timholy · 2020-10-22T14:38:15Z

Packages that would need [compat] bumps are just ImageFiltering, ImageMorphology, and Images (of those in General, anyway). Not bad at all. If we do want to increase the version number, perhaps we should contemplate 1.0?

jw3126 · 2020-10-22T15:41:47Z

Yeah, I think this is not breaking. I would go with 0.2.6. For 1.0 I think we might want to gather some more experience with the TileIterator(axes, strategy) API.

johnnychen94 · 2020-10-22T17:31:34Z

The only reason to bump to v0.3.0 is that Julia 0.7 support is dropped here.

timholy · 2020-10-22T18:26:13Z

Oh right, we'll have to bump in fact. OK, let's release this as 0.3.

jw3126 added 2 commits October 13, 2020 21:41

allow overlapping tiles

49911cb

add stride to TileIterator

00c773d

add tileiterator(axes, strategy)

af9bd17

jw3126 commented Oct 15, 2020

View reviewed changes

jw3126 added 3 commits October 16, 2020 08:04

drop julia v0.7 support

ddcac36

disable doctests on nightly

5de4087

add more docstrings

eff5af9

johnnychen94 reviewed Oct 19, 2020

View reviewed changes

src/tileiterator.jl Show resolved Hide resolved

jw3126 added 2 commits October 20, 2020 08:45

rename tileiterator -> TileIterator

98a43ad

drop julia 0.7 from appveyor

ea1fe04

johnnychen94 added the hacktoberfest-accepted label Oct 20, 2020

replace appveyor by travis

8a7c8bf

johnnychen94 reviewed Oct 20, 2020

View reviewed changes

src/tileiterator.jl Show resolved Hide resolved

test/runtests.jl Outdated Show resolved Hide resolved

test/runtests.jl Outdated Show resolved Hide resolved

src/tileiterator.jl Show resolved Hide resolved

jw3126 added 3 commits October 20, 2020 11:21

add more tests and docs

5d6dd20

fix TileIterator indexing

f12fd88

fix IdenityUnitRange not existing in old Julia

2fb3c96

johnnychen94 approved these changes Oct 20, 2020

View reviewed changes

timholy reviewed Oct 21, 2020

View reviewed changes

Update src/tileiterator.jl

5be49ac

Co-authored-by: Tim Holy <[email protected]>

jw3126 and others added 6 commits October 21, 2020 13:20

Update src/tileiterator.jl

4028549

Co-authored-by: Tim Holy <[email protected]>

Update src/tileiterator.jl

586e8f8

Co-authored-by: Tim Holy <[email protected]>

Update src/tileiterator.jl

802f7f9

Co-authored-by: Tim Holy <[email protected]>

Update src/TiledIteration.jl

6961da6

Co-authored-by: Tim Holy <[email protected]>

fix use of IdentityUnitRange

75b2a90

fix LengthAtMost

9abde07

fix

ee478a9

fix

528a7b1

timholy merged commit 5808ecf into JuliaArrays:master Oct 22, 2020

johnnychen94 mentioned this pull request Oct 23, 2020

add sliding_window JuliaImages/ImageFiltering.jl#155

Closed

This was referenced Nov 6, 2020

Implementing new IndexStyle / type of index? JuliaLang/julia#38284

Closed

Support arbitrarily offset tiling #25

Open

jw3126 mentioned this pull request Dec 3, 2020

WIP: rework TileIterator #27

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support overlapping tiles #23

Support overlapping tiles #23

jw3126 commented Oct 13, 2020

codecov bot commented Oct 13, 2020 •

edited

Loading

johnnychen94 commented Oct 14, 2020 •

edited

Loading

jw3126 commented Oct 14, 2020

johnnychen94 commented Oct 15, 2020

jw3126 commented Oct 15, 2020

johnnychen94 commented Oct 15, 2020 •

edited

Loading

jw3126 Oct 15, 2020

jw3126 Oct 15, 2020

jw3126 Oct 15, 2020

johnnychen94 Oct 19, 2020 •

edited

Loading

jw3126 Oct 20, 2020

johnnychen94 commented Oct 20, 2020 •

edited

Loading

jw3126 commented Oct 20, 2020

johnnychen94 left a comment

johnnychen94 commented Oct 20, 2020

johnnychen94 left a comment •

edited

Loading

jw3126 commented Oct 20, 2020

timholy commented Oct 20, 2020

timholy left a comment

timholy commented Oct 22, 2020

jw3126 commented Oct 22, 2020

timholy commented Oct 22, 2020

timholy commented Oct 22, 2020

timholy commented Oct 22, 2020

timholy commented Oct 22, 2020

jw3126 commented Oct 22, 2020

johnnychen94 commented Oct 22, 2020

timholy commented Oct 22, 2020

Support overlapping tiles #23

Support overlapping tiles #23

Conversation

jw3126 commented Oct 13, 2020

codecov bot commented Oct 13, 2020 • edited Loading

Codecov Report

johnnychen94 commented Oct 14, 2020 • edited Loading

jw3126 commented Oct 14, 2020

johnnychen94 commented Oct 15, 2020

jw3126 commented Oct 15, 2020

johnnychen94 commented Oct 15, 2020 • edited Loading

jw3126 Oct 15, 2020

Choose a reason for hiding this comment

jw3126 Oct 15, 2020

Choose a reason for hiding this comment

jw3126 Oct 15, 2020

Choose a reason for hiding this comment

johnnychen94 Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

jw3126 Oct 20, 2020

Choose a reason for hiding this comment

johnnychen94 commented Oct 20, 2020 • edited Loading

jw3126 commented Oct 20, 2020

johnnychen94 left a comment

Choose a reason for hiding this comment

johnnychen94 commented Oct 20, 2020

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

jw3126 commented Oct 20, 2020

timholy commented Oct 20, 2020

timholy left a comment

Choose a reason for hiding this comment

timholy commented Oct 22, 2020

jw3126 commented Oct 22, 2020

timholy commented Oct 22, 2020

timholy commented Oct 22, 2020

timholy commented Oct 22, 2020

timholy commented Oct 22, 2020

jw3126 commented Oct 22, 2020

johnnychen94 commented Oct 22, 2020

timholy commented Oct 22, 2020

codecov bot commented Oct 13, 2020 •

edited

Loading

johnnychen94 commented Oct 14, 2020 •

edited

Loading

johnnychen94 commented Oct 15, 2020 •

edited

Loading

johnnychen94 Oct 19, 2020 •

edited

Loading

johnnychen94 commented Oct 20, 2020 •

edited

Loading

johnnychen94 left a comment •

edited

Loading