Trying to simplify

[petebankhead]

This builds on top of #16 with the goal of trying to reduce boilerplate and make it easier to code for not knowing the image type - which is the 'usual' case in QuPath.

Some changes:

• Most methods in AccessibleScaler are very generic - so don't restrict their inputs so much (e.g. don't need a NativeType, and sometimes even NumericType)
• We never need to care about the second generic parameter for ImgBuilder<T,A> - so simplify it to ImgBuilder<T>
• Reduce the number and length of switch statements
• Add method for createRgbBuilder to go along with createRealBuilder

The last is to support this kind of code without the user having to know that ARGBType is what they need:

if (server.isRGB()) {
   // Handle RGB with known type
} else {
   // Handle everything else with real type
}

Then there's another change that I think will be more controversial:

• Add method public static <T extends RealType<T>> T getRealType(PixelType pixelType)

This is another generic method where the generic is only in the return value - so I documented why it isn't a good idea.

Still, the point of having it is to support this kind of code where the type is defined for the method:

public static <T extends RealType<T>> void doSomething() {
        try (var server = ImageServers.buildServer("")) {
            T typeGeneric = getRealType(server.getPixelType());
            RandomAccessibleInterval<T> imgGeneric = ImgBuilder.createBuilder(server, typeGeneric).buildForLevel(0);
            Views.collapseReal(imgGeneric);
        } catch (Exception e) {
            e.printStackTrace();
        }
}

I'm not sure this was possible with #16 because returning ImgBuilder<? extends NumericType<?>, ?> when the type wasn't known seemed too aggressive in throwing away information. Unless there was a safer syntax I missed, I think it pushes people towards using raw types or really large switch statements - both of which rather go against the point of Java generics and ImgLib2's purpose.

A big disadvantage is that it's possible to do something like this

FloatType type = ImgBuilder.getRealType(PixelType.UINT8);

however an exception should be thrown as soon as the type is used to create an image with an incompatible server, which I think mitigates the risk. At least it seems less dangerous than my previous attempts, which could allow this kind of thing

RandomAccessibleInterval<FloatType> img = ImgBuilder.createBuilder(server).buildForLevel(0)

and have the error show up much later.

---

The ultimate goal was to try to figure out some safer syntax that was also intuitive. This didn't really work out.
The method below is some code that I added at the end of ImgBuilder, which can be used to explore my attempts to see what did and didn't work (spoiler: most things don't work).

    public static <T extends RealType<T>> void doSomething(String[] args) {
            try (var server = ImageServers.buildServer("")) {
    
                // This works - use the generic parameter of the method
                T typeGeneric = getRealType(server.getPixelType());
                RandomAccessibleInterval<T> imgGeneric = ImgBuilder.createBuilder(server, typeGeneric).buildForLevel(0);
                Views.collapseReal(imgGeneric);
    
                // Doesn't compile - it's not ok to request real type inline
                var imgInline = ImgBuilder.createRealBuilder(server, getRealType(server.getPixelType())).buildForLevel(0);
                Views.collapseReal(imgInline);
    
                // Doesn't compile - need to specify a compatible type, RealType<?> isn't enough
                var imgUnsafe = ImgBuilder.createRealBuilder(server).buildForLevel(0);
                Views.collapseReal(imgUnsafe);
    
                // Doesn't compile - the fact we have a RealType doesn't manage to get through
                var imgUnreal = ImgBuilder.createBuilder(server, getRealType(server.getPixelType())).buildForLevel(0);
                Views.collapseReal(imgUnreal);
    
                // Doesn't compile - fails at buildForLevel (and we may well *not* have a FloatType)
                RandomAccessibleInterval<FloatType> imgDubious = ImgBuilder.createRealBuilder(server, getRealType(server.getPixelType())).buildForLevel(0);
                Views.collapseReal(imgDubious);
    
                // Doesn't compile - type only works if it's inline (for some reason)
                var typeUnknown = getRealType(server.getPixelType());
                var imgOutline = ImgBuilder.createRealBuilder(server, typeUnknown).buildForLevel(0);
                Views.collapseReal(imgOutline);
    
                // This compiles - but should fail when wrong type is passed to builder
                FloatType tf = getRealType(server.getPixelType()); // Might well be wrong!
                RandomAccessibleInterval<FloatType> img6 = ImgBuilder.createRealBuilder(server, tf).buildForLevel(0);
                Views.collapseReal(img6);
    
            } catch (Exception e) {
                e.printStackTrace();
            }
        }

I used Views.collapseReal because it has this signature:

public static < T extends RealType< T > > CompositeIntervalView< T, RealComposite< T > > collapseReal( final RandomAccessibleInterval< T > source )

and I wanted to see could be go from any arbitrary ImageServer to RandomAccessibleInterval<T> where we know that T is an instance of RealType but we don't know what exactly it is.

[alanocallaghan]

I confess to being quite lost at this point so don't feel very confident contributing. Is there code with a similar purpose to compare with somewhere in the imglib2/BDV/etc ecosystem? Or is it worth reaching out for some sanity-checking?

[petebankhead]

The closest thing I can think of it is in SCIFIO.open.

It looks like it isn't easy to handle the fact that you might not know what the type of the image you want is.

[petebankhead]

This is how I understand the issue... we want to be able to write ImgLib2 code and the challenge is to get our inputs in a convenient way.

I've given 3 examples below.
This PR tries to make the 1st easier, because it's concise. I'm not sure if it has major problems, but 2 doesn't feel good (and the compiler doesn't like it either), and 3 would be a lot of code in a lot of places.

// Define ImgLib2 function with whatever types are needed - the challenge is to figure out how to get a suitable input.
    public static <T extends NumericType<T> & NativeType<T>> void smoothAndShow(RandomAccessibleInterval<T> imgOrig) {
        // NumericType needed for smoothing, NativeType needed to create an ArrayImg (which we assume is small enough for memory)
        RandomAccessibleInterval<T> img2D = Views.hyperSlice(Views.dropSingletonDimensions(imgOrig), 2, 0);
        ArrayImg<T, ?> imgSmooth = new ArrayImgFactory<>(imgOrig.getType()).create(img2D.dimensionsAsLongArray());
        Gauss3.gauss(10.0, Views.extendMirrorSingle(img2D), imgSmooth);
        ImageJFunctions.show(imgSmooth);
    }

    // Option 1: Try to use generics, even when type not known
    public static <T extends RealType<T> & NativeType<T>> void smoothAndShow(ImageServer<BufferedImage> server) {
        T type = ImgBuilder.getRealType(server.getPixelType());
        RandomAccessibleInterval<T> imgOrig = ImgBuilder.createRealBuilder(server, type).buildForLevel(server.nResolutions()-1);
        smoothAndShow(imgOrig);
    }

    // Option 2: Use raw types to try to smuggle image through
    public static void smoothAndShowNonGeneric(ImageServer<BufferedImage> server) {
        var imgOrig = ImgBuilder.createRealBuilder(server).buildForLevel(server.nResolutions()-1);
        smoothAndShow((RandomAccessibleInterval)imgOrig);
    }

    // Option 3: Explicitly handle types
    public static void smoothAndShowAwkward(ImageServer<BufferedImage> server) {
        int level = server.nResolutions()-1;
        switch (server.getPixelType()) {
            case UINT8 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new UnsignedByteType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case INT8 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new ByteType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case UINT16 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new UnsignedShortType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case INT16 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new ShortType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case UINT32 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new UnsignedIntType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case INT32 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new IntType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case FLOAT32 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new FloatType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
            case FLOAT64 -> {
                var imgOrig = ImgBuilder.createBuilder(server, new DoubleType()).buildForLevel(level);
                smoothAndShow(imgOrig);
            }
        }
    }

[alanocallaghan]

The scifio code doesn't look a million miles from what we've got here. As long as we're fairly sure that exceptions will be (fairly) local to the code that causes them, then I'm mostly happy (but still very lost).

Re:

FloatType tf = getRealType(server.getPixelType());

I guess we just chalk this up to user error? The nested generic types (which still do not really make sense to me) seem to make some level of unchecked type use inevitable; I certainly recall somebody advising us to just use @suppresswarnings("unchecked") somewhere

[Rylern]

I think the decrease in number of lines of code and the simplification of the public API compensates the overall use of unsafe functions. I just have a few comments that to simplify even more

[petebankhead]

Thanks @Rylern and @alanocallaghan I think I've fixed/replied to everything.

I made some other changes to try to improve reuse/limit potential code paths, for example by restricting calls to the ImgBuilder constructor to the createBuilder(server, type) method only.

[Rylern]

Minor things