Skip to content
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

fluent API for chaining views #364

Merged
merged 35 commits into from
Aug 9, 2024
Merged

fluent API for chaining views #364

merged 35 commits into from
Aug 9, 2024

Conversation

tpietzsch
Copy link
Member

Updated version of #348

Add a fluent API for chaining Views methods on RandomAccessibleInterval etc.
This would look something like:

Img<IntType> img = ...
RealRandomAccessible<IntType> interpolated = img.view()
		.slice(2, 0)
		.permute(0, 1)
		.extend(border())
		.interpolate(nLinear());

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/recent-and-upcoming-imglib2-improvements/96083/1

@tpietzsch
Copy link
Member Author

(copied from forum post https://forum.image.sc/t/recent-and-upcoming-imglib2-improvements/96083/1)

There are some open questions about naming things, API design choices, etc. Looking forward to opinions and suggestions!

It works like this:
There are three new interfaces RaView<T,?>, RaiView<T>, and RraView<T> that implement and wrap respectively RandomAccessible<T>, RandomAccessibleInterval<T>, and RealRandomAccessible<T>. They also expose Views and Converters methods that work on the respective Accessible. The results of these methods are wrapped again in RaView<T,?>, RaiView<T>, or RraView<T>, to allow for chaining operations. To get one of the wrappers in the first place, RandomAccessible<T> etc have a new view() method (with default implementation).

Design

One key goal is better discoverability than with the static Views methods. (There are too many. And you have to know about the existence of Views in the first place). When you type img.view(). then your IDE should give helpful suggestions. Having distinct wrapper interfaces for each Accessible allow to limit the method suggestions to those applicable in the current context. I tried to limit the API to the most used Views and Converters methods.

.view() gateway

The current approach is that img.view() gives you a gateway to the Views methods. Another option would be to add the views methods to the respective Accessible interface directly which would avoid the initial .view(). I prefer the current gateway-behind-a-method approach over polluting the Accessible interfaces with many more methods. But this is up for debate.

extend/expand API

For modeling Views.extend... and Views.expand... methods, there are two issues.

First, putting them all in the RaiView interface would add a lot of similarly named methods.

extendMirrorSingle()
extendMirrorDouble()
extendPeriodic()
extendBorder()
extendZero()
extendValue(T value)
expandMirrorSingle(long... border)
expandMirrorDouble(long... border)
expandPeriodic(long... border)
expandBorder(long... border)
expandZero(long... border)
expandValue(T value, long... border)

This makes discoverability of the API through IDE auto-completion a bit worse.

Second, it is not technically correct to include extendZero() in the RaiView<T> interface because it does not work for all T. For example RaiView<String>.extendZero() should not work. However, extendZero() is one of the most used Views extensions, so it would be a pity to leave it out.

I decided to solve this as follows: The two methods, extend(...) and expand(...) take a RaiView.Extension argument, which contains the OutOfBoundsFactory to use. RaiView.Extension can only be created through static constructor methods.
It is supposed to be used like this:

RandomAccessibleInterval<IntType> img;
img.view().extend(Extension.border());
img.view().extend(Extension.zero());

etc. This puts the extension variants behind another API layer, leaving only the two top-level methods extend() and expand(). With RaiView.Extension only instantiable through factory methods, and the pattern pointed out in the javadoc, this should be pretty easily discoverable.
Moreover, the factory methods can specify generic bounds, so for example

RandomAccessibleInterval<String> strings;
RandomAccessibleInterval<IntType> ints;

// this works:
ints.view().extend(Extension.zero());

// this does not work:
// compile error: no instance(s) of type variable(s) T exist so that String conforms to Type<T>
strings.view().extend(Extension.zero());

For consistency, the same API style is used for interpolation, e.g., .interpolate(Interpolation.nLinear()).

Extensibility

Ideally we would be able to incorporate RealViews methods into the RraView interface. However, these live in a separate repository and therefore we cannot use them imglib2 core.

We briefly considered merging the imglib2-realviews and imglib2 repositories. But it doesn't end there. Maybe we want to have a method like .cache() which would require imglib2-cache, and so on.
The solution for now is a method .apply(Function<?,?>), which can act as a general "escape hatch" that can be used to support the above use cases (and others).

Unfortunately, this requires a generic parameter on RaView to be able to ensure type-safety.

interface RaView< T, V extends RaView< T, V > > extends RandomAccessible< T >
{   ...
    default < U > U apply( Function< ? super V, U > function ) {
        return function.apply( ( V ) this );
    }
}

public interface RaiView< T > extends RaView< T, RaiView< T > >, RandomAccessibleInterval< T >
{   ...
}

With the above, we have as desired:

Img<?> img = ...;
Function<RandomAccessible<?>, String>         fn1 = ra  -> "accept RandomAccessible";
Function<RandomAccessibleInterval<?>, String> fn2 = rai -> "accept RandomAccessibleInterval";

// these work:
img.view().apply( fn1 );
img.view().apply( fn2 );
img.view().extend( Extension.border() ).apply( fn1 );

// this does not work:
// compile error: java.util.function.Function<net.imglib2.RandomAccessibleInterval<?>,java.lang.String>
//                cannot be converted to java.util.function.Function<? super capture#1 of ?,U>)
img.view().extend( Extension.border() ).apply( fn2 );

There are many ways this apply() idea could be built out. Imagine "function factories" that create apply() Functions.
I sketched a few examples here.

RealViewsExample shows what a factory for RealViews transformations could look like.

RandomAccessibleInterval< IntType > affine1 = img.view()
        .extend( Extension.border() )
        .interpolate( Interpolation.nLinear() )
        .apply( transform )
        .interval( img );

RandomAccessibleInterval< IntType > affine2 = img.view()
        .extend( Extension.border() )
        .interpolate( Interpolation.nLinear() )
        .apply( affine2D().scale( 1.1 ).translate( 2, 1.3 ).transform() )
        .interval( img );

(Look at the full example for how affine2D() is defined.)

MaterializeExample shows how Collector.toList()-style terminal operations could be implemented.

ArrayImg< UnsignedByteType, ? > img = function.view()
        .interval( new FinalInterval( 4, 4 ) )
        .expand( mirrorSingle(), 2 )
        .zeroMin()
        .apply( toArrayImg() );

CachedCellImg< UnsignedByteType, ? > lazy = function.view()
        .interval( new FinalInterval( 4, 4 ) )
        .expand( mirrorSingle(), 2 )
        .zeroMin()
        .apply( toCachedCellImg( 3 ) );

(Again, look at the full example for how toArrayImg() and toCachedCellImg() are defined.)

The cool thing about these "function factory" methods is that they can impose type constraints. For example toArrayImg() can only be passed to apply() of a view of NativeType. This does not compile:

Img< String > strings = new ListImg<>( new long[] { 1 }, "" );
strings.view().apply( toArrayImg() );
// compile error: no instance(s) of type variable(s) T exist so that String conforms to NativeType<T>

Naming

  • I named the slice() method slice instead of hyperSlice.
  • I'm not sure what the above apply() method should be called. Other options are transform(), map(), or something completely different?

Opinions?

@minnerbe
Copy link
Contributor

minnerbe commented May 9, 2024

That's awesome! 🚀 Thanks @tpietzsch for following through with that idea!

I really like having the .view() gateway. First, since it doesn't pollute the existing interfaces (which you already mentioned). Second, I like the analogy with streams. That way, you can create a view from something that is not already an image / accessible to quickly create an image (think IntStream.range(0, 10)).

Similarly, the Extension and Interpolation mechanisms closely mimic the Collectors API, so it should be familiar to most people. A small semantic difference is that you name the interfaces like actions, whereas the collectors API names them like actors. There are probably good arguments for the collectors nomenclature, but I slightly prefer your way.

The examples you cooked up are super expressive! I haven't had time to look at the code in detail yet, but I think the apply method is a bit too general: it's used to transform views, but also to materialize views into images (for which there are also other methods). In particular, it is not totally clear to me at first glance where views end and intervals start. I hope to get a closer look within the next few days.

@tpietzsch
Copy link
Member Author

Similarly, the Extension and Interpolation mechanisms closely mimic the Collectors API, so it should be familiar to most people. A small semantic difference is that you name the interfaces like actions, whereas the collectors API names them like actors. There are probably good arguments for the collectors nomenclature, but I slightly prefer your way.

The actors nomenclature would mean calling it Extenders and Interpolators?

The examples you cooked up are super expressive! I haven't had time to look at the code in detail yet, but I think the apply method is a bit too general: it's used to transform views, but also to materialize views into images (for which there are also other methods). In particular, it is not totally clear to me at first glance where views end and intervals start. I hope to get a closer look within the next few days.

There is no other materialize methods. Maybe the confusing bit is that RaView<T> also extends RandomAccessible<T> etc? So

RandomAccessibleInterval< IntType > affine1 = img.view()
        .extend( Extension.border() )
        .interpolate( Interpolation.nLinear() )
        .apply( transform )
        .interval( img );

may look like the interval() is a materializing method, but that's just a RaiView<IntType>.
I assigned it to a RandomAccessibleInterval<IntType> variable, because I didn't want to encourage to use RaiView as a variable or argument type.

@minnerbe
Copy link
Contributor

The actors nomenclature would mean calling it Extenders and Interpolators?

Exactly. This just seems a bit clunky to me.

Maybe the confusing bit is that RaView also extends RandomAccessible etc?

Yes, I think that's what confused me. Is there any downside to not having RaiView extend RandomAccessibleInterval? Or, more fundamentally, are *RandomAccessible*s ever used in a non-view sense (i.e., without lazy computation)?

I guess what is bothering me is that there is no clear separation of lazily computed views and materialized images, which is important since both have very different access cost. Maybe just having an explicit materialize() method that takes toArrayImg() and so on instead of having this handled by apply() is already clear enough?

@tpietzsch
Copy link
Member Author

The actors nomenclature would mean calling it Extenders and Interpolators?

Exactly. This just seems a bit clunky to me.

Yes, agreed. Interpolators would be ok, but Extenders is weird.

Maybe the confusing bit is that RaView also extends RandomAccessible etc?

Yes, I think that's what confused me. Is there any downside to not having RaiView extend RandomAccessibleInterval? Or, more fundamentally, are *RandomAccessible*s ever used in a non-view sense (i.e., without lazy computation)?

I guess what is bothering me is that there is no clear separation of lazily computed views and materialized images, which is important since both have very different access cost. Maybe just having an explicit materialize() method that takes toArrayImg() and so on instead of having this handled by apply() is already clear enough?

Yes, *RandomAccessible*s are used in a non-view sense all the time. It is a fundamental point of ImgLib2 to not make that distinction. The difference in access cost is also not clear-cut. For example, a RandomAccess on a hyperslice is probably not more expensive than the non-sliced RandomAccess, the RandomAccess of an IntervalView is just the un-modified access of the underlying RandomAccessible. I'm not sure whether a translated and axis-permuted view of an ArrayImg is more or less expensive than a CellImg, etc.

@minnerbe
Copy link
Contributor

Yes, RandomAccessibles are used in a non-view sense all the time. It is a fundamental point of ImgLib2 to not make that distinction.

I see, thanks for the clarification! Still, I think that having a materialize() method separate from the catch-all apply could be worthwhile.

@tpietzsch tpietzsch force-pushed the streamify-views branch 2 times, most recently from c15b51d to 425e399 Compare May 15, 2024 21:19
minnerbe and others added 20 commits June 12, 2024 18:11
This is necessary to make apply(Function) work correctly for both, RaView
and RaiView. Effectively, the function given to RaView.apply() should
take RaView (or any parent, e.g., RandomAccessible). The function given to
RaiView.app;y() should take RaiView (or any parent, e.g., RandomAccessibleInterval).

I can find no other way to make this work.
Hopefully this is not too inconvenient because RaView is not supposed to be
used as type of variables, etc. It is just a construct to make the view-
chaining API possible
chains
(also handles RaiView)
The view gateways implement the respective interfaces and we want people to
use RealRandomAccessibleInterval instead of RraView for variable types.
Having an even longer name is hopefully a little push in that direction.
@tpietzsch tpietzsch merged commit e71d4f5 into master Aug 9, 2024
1 check passed
@tpietzsch tpietzsch deleted the streamify-views branch August 9, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants