-
Notifications
You must be signed in to change notification settings - Fork 93
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.
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
Add support for localizing LoopBuilders #273
Conversation
I can think of two alternatives that I would prefer to make this more general, that could be even implemented both. 1.) We could add a convenience method maybe to Instead of LoopBuilder.setImagesLocalizing( imageA, imageB )
.forEachPixel( (l, a, b) -> {...} ); it would be LoopBuilder.setImages( Intervals.positions(imageA), imageA, imageB )
.forEachPixel( (l, a, b) -> {...} ); Not much more to write, but now you can choose whether to provide coordinates of 2.) LoopBuilder.setImagesLocalizing( imageA, imageB )
.forEachPixel( (l, a, b) -> {
...
} ); it would be LoopBuilder.setImages( imageA, imageB )
.forEachPixelLocalizing( (ca, cb) -> {
Localizable l = ca;
A a = ca.get();
B b = cb.get();
...
} ); |
4606e57
to
f80f294
Compare
Thanks @tpietzsch for your comments! (1) is a good idea, and already exists. See I really like (2), and had considered doing it that way when starting this work. But I got hung up on the fact that the current generic typing ( I have now updated the PR to work as you describe in (2). Support for all six arities is included, so the commit is no longer a WIP. I overcame the above typing issue by introducing a second generic parameter I considered generalizing this to
Under what conditions would a different coordinate be provided for |
292f0b7
to
5d31666
Compare
I've now added arities 7 through 10. @etarena and I were actually trying to loop over 8 synced images and ran out of |
I think,
|
oh oh, looks like I'm losing track of things ... :-) |
We want to leave the door open to internal use of localizing cursors, rather than explicitly promising RandomAccesses always. See: #273 (comment)
I changed it: 107342e This will prevent people from being able to reposition the samplers as easily since the static typing will not expose that they are |
@tpietzsch Are we OK to merge it? |
I would wait for @maarzt to review, if it is not urgent. |
Sure. Though he'd be within his rights to demand a quid pro quo for scijava/scijava-ui-swing#43 😆 |
I like the solution (1), that @tpietzsch proposed earlier. It solves many potential use cases and is easy to understand. Plus, the code is very readable:
We would only need to add the The implementation of the |
Regarding the second problem with more than six images. @etadobson I don't know the actual use case but I guess many of the 7 images are similar. It might be feasible to use
to merge the similar images into one image. Or if you are working with neighborhoods |
{ | ||
action.accept( accessA, accessB, accessC, accessD, accessE, accessF, accessG, accessH, accessI, accessJ ); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of LoopBuilder has already many lines of code. I fear that adding these 500+ lines of code reduces the maintainability. Let us go for this shorter solution #279.
Closed in favor of #279 |
Following on to @etarena's question in the Gitter channel the other day ("Is there a way to use LoopBuilder (or something similar) to loop multiple things while knowing their position?"), I looked into what it would take to add support for
LoopBuilder
loops that include aLocalizable
.Turns out it was pretty straightforward! Here is a very quick first cut. Comments:
setImagesLocalizing
) but just picked something to move forward. I don't feel strongly about it.allCursorsAreFast
check and soLoopBuilder
will fall back to random accesses anyway.LoopBuilder
design, mostly it is quite elegant! But I got a bad feeling about the fact that the*ConsumerRunnable
classes hardcodeSampler
arguments and calls toSampler.get()
when they could potentially be either A) more general or B) more specifically named to indicate that.Feedback on whether this is a thing we want to have? Did I miss an already existing way to do this? It would have been helpful on a recent project with @etarena where we had to loop several images in sync, while needing to localize them at each point...