-
Notifications
You must be signed in to change notification settings - Fork 95
Propagate RAView type variables to RAIView, RRAView #379
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
base: master
Are you sure you want to change the base?
Conversation
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/fiji-friends-weekly-dev-update-thread/103718/99 |
|
This could work, I think. Did you / could you please check that this does not break any of the discoverability and type-safety features discussed in https://forum.image.sc/t/recent-and-upcoming-imglib2-improvements/96083?u=tpietzsch? Can you explain what wouldn't work here without #378? |
Ooh, actually, this may work as is without #378. What wouldn't work is the following, which I hadn't yet added but would make a lot of sense if we did go with #378: @@ -72,7 +70,7 @@ import net.imglib2.view.Views;
* @author Michael Innerberger
* @see Views
*/
-public interface RealRandomAccessibleView< T, V extends RealRandomAccessibleView<T, V> > extends RealRandomAccessible< T >
+public interface RealRandomAccessibleView< T, V extends RealRandomAccessibleView<T, V> > extends RandomAccessibleView<T, V>, RealRandomAccessible< T >
{
RealRandomAccessible< T > delegate();
Can you be more specific about where you're concerned? Happy to write tests to ensure proper behavior. |
68b60ca to
56a62b1
Compare
56a62b1 to
19950b5
Compare
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.
Pull Request Overview
This PR propagates the self-referential type variable pattern from RandomAccessibleView to RandomAccessibleIntervalView and RealRandomAccessibleView. This enables better type safety and allows methods like use() to properly preserve the actual view type.
Key Changes:
- Added second type parameter
VtoRealRandomAccessibleViewandRandomAccessibleIntervalViewinterfaces using self-referential bounds - Updated all method return types to use wildcard
?for the new type parameter where the concrete type cannot be preserved - Modified
Extensioninner class to include the type parameter - Removed redundant
use()method override fromRandomAccessibleIntervalView(now inherited from parent)
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| RealRandomAccessibleView.java | Added self-referential type parameter V and updated method signatures to propagate it |
| RandomAccessibleView.java | Updated wrap() return type and child view method return types to use wildcard |
| RandomAccessibleIntervalView.java | Added type parameter, updated Extension class, removed redundant use() override, changed imports |
| RealRandomAccessible.java | Updated realView() return type to use wildcard |
| RandomAccessibleInterval.java | Updated view() return type to use wildcard |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/net/imglib2/view/fluent/RealRandomAccessibleView.java
Outdated
Show resolved
Hide resolved
src/main/java/net/imglib2/view/fluent/RandomAccessibleIntervalView.java
Outdated
Show resolved
Hide resolved
src/main/java/net/imglib2/view/fluent/RandomAccessibleIntervalView.java
Outdated
Show resolved
Hide resolved
|
@tpietzsch I did another pass over the code. Couple small fixes to formatting as well as a bug or two fixed 🛠️
None of the features discussed in that forum post you linked seem to be broken there. In fact, we get the same behavior, but can remove the explicit package net.imglib2.realtransform;
import java.util.function.Function;
import net.imglib2.RandomAccessibleInterval;
import net.imglib2.RealRandomAccessible;
import net.imglib2.img.array.ArrayImgs;
import net.imglib2.type.numeric.integer.IntType;
import net.imglib2.view.fluent.RandomAccessibleView;
import net.imglib2.view.fluent.RandomAccessibleView.Interpolation;
import net.imglib2.view.fluent.RandomAccessibleIntervalView.Extension;
import net.imglib2.view.fluent.RealRandomAccessibleView;
public class RealViewsExample
{
public static void main( String[] args )
{
RandomAccessibleInterval< IntType > img = ArrayImgs.ints( 100, 100 );
// Arbitrary functions can be passed to RraView.apply().
//
// Here we create a Function that transforms a RealRandomAccessible with
// a AffineTransform2D (using RealViews.affine) and returns a RaView of the result.
AffineTransform2D affineTransform2D = new AffineTransform2D();
affineTransform2D.scale( 1.1 );
affineTransform2D.translate( 2.1, 1.3 );
Function< RealRandomAccessible< IntType >, RandomAccessibleView< IntType, ? >> transform = rra ->
RandomAccessibleView.wrap( RealViews.affine( rra, affineTransform2D ) );
// We can use that in RraView.apply(). Our function returns a RaView, so
// we can keep chaining more methods on the result.
RandomAccessibleInterval< IntType > affine1 = img.view()
.extend( Extension.border() )
.interpolate( Interpolation.nLinear() )
.use( transform )
.interval( img );
// --------------------------------------------------------------------
// We can think about creating "function factories" like the Affine2D
// prototype below.
//
// The above can then be written like this:
RandomAccessibleInterval< IntType > affine2 = img.view()
.extend( Extension.border() )
.interpolate( Interpolation.nLinear() )
.use( Affine2D.affine2D().scale( 1.1 ).translate( 2, 1.3 ).transform() )
.interval( img );
// Here affine2D creates a builder.
// scale() and translate() concatenate transformations.
// Finally, transform() creates a function to be used in apply().
// --------------------------------------------------------------------
// transformReal() creates another function to be used in apply() This
// one uses RealViews.affineReal, so the result is a RraView instead of
// a RaView.
RealRandomAccessible< IntType > affine3 = img.view()
.extend( Extension.border() )
.interpolate( Interpolation.nLinear() )
.use( Affine2D.affine2D().scale( 1.1 ).translate( 2, 1.3 ).transformReal() );
System.out.println(affine3.getAt(0.0, 0.0));
}
public static class Affine2D
{
private final AffineTransform2D transform;
public Affine2D( AffineTransform2D transform )
{
this.transform = transform;
}
public static Affine2D affine2D()
{
return new Affine2D( new AffineTransform2D() );
}
public Affine2D translate( double... txy )
{
final AffineTransform2D t = transform.copy();
t.translate( txy );
return new Affine2D( t );
}
public Affine2D rotate( double d )
{
final AffineTransform2D t = transform.copy();
t.rotate( d );
return new Affine2D( t );
}
public Affine2D scale( double s )
{
final AffineTransform2D t = transform.copy();
t.scale( s );
return new Affine2D( t );
}
public < T > Function< RealRandomAccessible< T >, RandomAccessibleView< T, ? > > transform()
{
return rra -> RandomAccessibleView.wrap( RealViews.affine( rra, transform ) );
}
public < T > Function< RealRandomAccessible< T >, RealRandomAccessibleView< T, ? > > transformReal()
{
return rra -> RealRandomAccessibleView.wrap( RealViews.affineReal( rra, transform ) );
}
}
}Happy to address any further concerns you may have here! |
This PR is built atop #378 - in many ways, these changes motivated the changes in that PR. Therefore, it should be reviewed first. It also warrants a major version bump.
Closes #377