imglib / imglib2

A generic next-generation Java library for image processing
http://imglib2.net/
Other
293 stars 93 forks source link

Create LocalizableSamplerStream #338

Closed tpietzsch closed 8 months ago

tpietzsch commented 10 months ago

With Img.stream(), it is limiting to have only access to pixel values. It would be more useful to also know pixel positions.

For this, ImgLib2 should provide Stream<LocalizableSampler<T>> in addition to Stream<T>.

The implementation is rather straightforward: basically just modify the RealCursorSpliterator to use Cursor<T> instead of Cursor<T>.get() for the stream elements.

Figuring out the API is not straightforward unfortunately.

tpietzsch commented 10 months ago

Originally posted by @ctrueden in https://github.com/imglib/imglib2/issues/336#issuecomment-1716011621

For the localizable stream elements: I like this idea. The method could just be .localizingStream() for symmetry with localizingCursor(), eh?

Hmm, unfortunately that could be misguiding. cursor() and localizingCursor() both return Cursor<T>, but stream() and localizingStream() would return Stream<T> and Stream< LocalizableSampler> respectively.

Although I guess we probably also want .localizingParallelStream() 🙄 ... But then as you say, the generics get tough. Maybe instead of baking it into the IterableRealInterval interface, some static utility methods would be easier? Like:

public static < T > Stream< RealCursor< T >> localizingStream( IterableRealInterval< T > iri ) { ... }
public static < T > Stream< Cursor< T >> localizingStream( IterableInterval< T > ii ) { ... }

This avoids the hairiness of incompatible return type of an overridden method in IterableInterval due to non-covariance.

Yes, the non-covariance is one problem.

Additionally, I would like to have the return type as something like Stream<Localizable & Sampler<T>>, so that one cannot call fwd() or next() on the stream elements.

Unfortunately, one cannot specify Localizable & Sampler<T> like this. We could add combined interfaces

interface RealLocalizableSampler<T> extends RealLocalizable, Sampler<T>
{}

interface LocalizableSampler<T> extends Localizable, RealLocalizableSampler<T> 
{}

and use them in the appropriate places (although we always wanted to avoid these combined interfaces). With that, I got as far as

public interface IterableRealInterval<T> extends RealInterval, Iterable<T> {
    // ...

    Spliterator<? extends RealLocalizableSampler<T>> samplerSpliterator();

    default Stream<? extends RealLocalizableSampler<T>> samplerStream() {
        return StreamSupport.stream(samplerSpliterator(), false);
    }
}

public interface IterableInterval<T> extends IterableRealInterval<T>, Interval {
    @Override
    Spliterator<LocalizableSampler<T>> samplerSpliterator();

    @Override
    default Stream<LocalizableSampler<T>> samplerStream() {
        return StreamSupport.stream(samplerSpliterator(), false);
    }
}

I like the static utility methods approach! The only downside is that it is harder to special-case for ArrayImg etc.

I'll play around a bit more...

ctrueden commented 10 months ago

so that one cannot call fwd() or next() on the stream elements.

A compromise could be to use anonymous subclasses of Cursor/RealCursor that throw UnsupportedOperationException for the operations that should really not be done while streaming. Then people will at least get fail-fast behavior, if not compile-time notification of errors.

ctrueden commented 10 months ago

Oh, a more radical solution would be: don't use Cursor/RealCursor at all for the localizing stream elements. Use a new thing, which implements just the interfaces you want. Of course, it has a lot of duplication with Cursor...

tpietzsch commented 10 months ago

Aha! I think I have something quite nice!

Similarly to Cursor and RealCursor, I'll make the Spliterator itself carry position information.

public interface RealLocalizableSpliterator<T> extends Spliterator<T>, RealLocalizable
{
    @Override
    RealLocalizableSpliterator< T > trySplit();
}

public interface LocalizableSpliterator<T> extends RealLocalizableSpliterator<T>, Localizable
{
    @Override
    LocalizableSpliterator< T > trySplit();
}

It is then relatively straightforward to make wrappers

public class RealLocalizableSamplerWrapper<T> implements Spliterator<RealLocalizableSampler<T>>
{
    public RealLocalizableSamplerWrapper(RealLocalizableSpliterator<T> delegate) {
        ...
    }
    ...
}

class LocalizableSamplerWrapper<T> implements Spliterator<LocalizableSampler<T>>
{
    public LocalizableSamplerWrapper(LocalizableSpliterator<T> delegate) {
        ...
    }
    ...
}

and static utility methods as @ctrueden suggested:

public class Streams
{
    public static <T> Stream<RealLocalizableSampler<T>> localizing(IterableRealInterval<T> interval) {
        return StreamSupport.stream(new RealLocalizableSamplerWrapper<>(interval.spliterator()), false);
    }

    public static <T> Stream<LocalizableSampler<T>> localizing(IterableInterval<T> interval) {
        return StreamSupport.stream( new LocalizableSamplerWrapper<>(interval.spliterator()), false );
    }

I like the static utility methods approach! The only downside is that it is harder to special-case for ArrayImg etc. Not anymore. Now ArrayImg can just provide a specialized LocalizableSpliterator<T>

The best thing is that (Real)LocalizableSpliterator<T> are still also Spliterator<T>. So there can be just one Iterable(Real)Interval.spliterator() method.

public interface IterableRealInterval<T> extends RealInterval, Iterable<T>
{
    @Override
    RealLocalizableSpliterator<T> spliterator();

    default Stream<T> stream() {
        return StreamSupport.stream(spliterator(), false);
    }
    ...
}

public interface IterableInterval<T> extends IterableRealInterval<T>, Interval 
{
    @Override
    RealLocalizableSpliterator<T> spliterator();
    ...
}