Simplify ImgCreator by reducing use of NativeType and generic parameters

[petebankhead]

This tries to substantially reduce the complexity of ImgCreator.

The need arose when I was finding it really hard to manage generic parameters of the form <T extends NativeType<T> & NumericType<T>>.

Many methods don't/shouldn't care that we have a NativeType, but rather only that we have a NumericType.

After removing NativeType from some locations where it didn't seem needed, it became clear that ImgCreator doesn't really need to store the type at all. Rather, the type can be determined from the ImageServer at the point the image is requested.

That means it's no longer necessary to provide a type to ImgCreator.Builder - making it easier to call.

Internally we do still need to always use a NativeType so that we can create a LazyCellImage. This means we can retain the method signature:

public <T extends NumericType<T> & NativeType<T>> Img<T> createForLevel(int level)

so calling code can be confident that the type is both a NumericType and a NativeType - treating it as either.

However I'm not sure if there are risks to this, which undermine compile-time checks. For example the following code looks ok in my IDE:

@Test
    void Check_Rgb_Server() throws Exception {
        boolean isRgb = true;
        PixelType pixelType = PixelType.UINT8;
        try (var imageServer = new GenericImageServer(isRgb, pixelType)) {
            Img<UnsignedByteType> img = ImgCreator.builder(imageServer).build().createForLevel(0);
            Utils.assertRandomAccessibleEquals(img, (x, y, channel, z, t) -> 255, 1);
        }
    }

However it fails at runtime with

class net.imglib2.type.numeric.ARGBType cannot be cast to class net.imglib2.type.numeric.RealType (net.imglib2.type.numeric.ARGBType and net.imglib2.type.numeric.RealType are in unnamed module of loader 'app')
java.lang.ClassCastException: class net.imglib2.type.numeric.ARGBType cannot be cast to class net.imglib2.type.numeric.RealType (net.imglib2.type.numeric.ARGBType and net.imglib2.type.numeric.RealType are in unnamed module of loader 'app')
	at qupath.ext.imglib2.Utils.assertRandomAccessibleEquals(Utils.java:106)
	at qupath.ext.imglib2.TestImgCreator.Check_Rgb_Server(TestImgCreator.java:47)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1604)

I'm not sure if this is a limitation of the way I'm using Java generics here (and we simply need to be checking the type that's returned), or if there's any way we can improve the API further. We would already have had a problem with the previous code if we passed the wrong type, so I don't think we're worse off here.

[petebankhead]

Upon reflection, I don’t think this PR goes far enough. It feels strange that we use a Builder to build an ImgCreator to create an Img (or a RandomAccessibleInterval). The distinction between building and creating isn’t obvious to me.

Because the Builder now only takes an ImageServer and an optional CellCache, I don’t think the builder pattern is needed. We can just use static methods, e.g.

public static ImgCreator fromServer(ImageServer server) {…}
public static ImgCreator fromServer(ImageServer server, CellCache cache) {…}

There should be a way to query the type that will be used for an ImageServer (e.g. make getTypeOfServer public).

We could then consider adding separate methods such as

public <T extends NativeType<T> & NumericType<T>> Img<T> createForLevel(int level, T type) {…}

so that the type could explicitly be provided. If we do that, then we need to decide whether we permit lossless conversions (e.g. UnsignedByteType to FloatType, but not the other way around).

The most important distinction is likely between RealType and ARGBType. There might be createForLevelARGB or createForLevelReal convenience methods.

[Rylern]

The whole point of the current design was to have a safe function that avoids these ClassCastExceptions. With this PR, getTypeOfServer() (which has a lot of Unchecked cast) is always called, whereas in the previous implementation it was only called with the unsafe function. The safe function was throwing IllegalArgumentException in case the provided type was invalid, and I preferred that because it meant no compilation warnings. Also, one ImgLib2 example has a similar behaviour of providing the type when opening an image.
I think it's OK if there are no safe functions and thus potential ClassCastException instead of IllegalArgumentException, but in any case this should be documented in the javadoc.

About defining the type in the builder vs defining it in the functions, since the type was entirely determined by the provided ImageServer, I thought it was logical to defining it when the ImageServer is provided, not after:

• When you call the builder, you provide the ImageServer, so you know the type.
• When you call e.g. createForLevel(), you might not have access to the ImageServer that was used to create the instance of ImgCreator, so you might not know the type (which is unlikely I agree).

What are the advantages of defining the type in the functions?

Because the Builder now only takes an ImageServer and an optional CellCache, I don’t think the builder pattern is needed

Having a builder makes the API more future-proof, but if we are sure that we won't add any new parameters, then yes we can go with fromServer().

There should be a way to query the type that will be used for an ImageServer (e.g. make getTypeOfServer public).

Did you try it in code? It's something I initially tried to do, but it wasn't working / needed a lot of unchecked casts.

We could then consider adding separate methods such as

public <T extends NativeType<T> & NumericType<T>> Img<T> createForLevel(int level, T type) {…}

But then doesn't it become quite similar to the code before this PR, with the same complexity on the calling side?

[petebankhead]

The whole point of the current design was to have a safe function that avoids these ClassCastExceptions. With this PR, getTypeOfServer() (which has a lot of Unchecked cast) is always called, whereas in the previous implementation it was only called with the unsafe function. The safe function was throwing IllegalArgumentException in case the provided type was invalid, and I preferred that because it meant no compilation warnings.

Is that any better at runtime though? How would the user know which type to provide, unless they are essentially replicating every time what getTypeOfServer() is doing to check the PixelType?

Also, one ImgLib2 example has a similar behaviour of providing the type when opening an image.

That's ok for an example, but won't help us much in practice. In general we won't know what the type is for an image that hasn't been opened.

I think it's OK if there are no safe functions and thus potential ClassCastException instead of IllegalArgumentException, but in any case this should be documented in the javadoc.

Yes, we should document the behavior.

My last suggestion was to provide both options: where a type is provided, and where either isn't (and perhaps adding a specific alternative for ARGB, since it is a special case - and can be checked in a different way using ImageServer.isRGB()). For example

public <T extends NumericType<T> & NativeType<T>> Img<T> createForLevel(int level) {
        T type = getTypeOfServer(server);
        return createForLevel(level, type);
    }

    public Img<ARGBType> createForLevelARGB(int level) {
        var type = getTypeOfServer(server);
        if (type instanceof ARGBType argb) {
            return createForLevel(level, argb);
        } else {
            throw new IllegalArgumentException("Can't create ARGB Img from a non-RGB ImageServer");
        }
    }

    public <T extends NumericType<T> & NativeType<T>> Img<T> createForLevel(int level, T type) {
        if (level < 0 || level >= server.getMetadata().nLevels()) {
       //.... Everything else
    }

Having a builder makes the API more future-proof, but if we are sure that we won't add any new parameters, then yes we can go with fromServer().

Do you already have ideas about additional parameters to add?
If not, I think static methods would simplify use considerably. And we can always add a builder later if it becomes necessary.

There should be a way to query the type that will be used for an ImageServer (e.g. make getTypeOfServer public).

Did you try it in code? It's something I initially tried to do, but it wasn't working / needed a lot of unchecked casts.

I think I'd need an example to understand what exactly isn't working.

But then doesn't it become quite similar to the code before this PR, with the same complexity on the calling side?

Maybe... I think it has some advantages, but we probably need more code examples to investigate.

My starting point was code related to thresholding that looked very repetitive. I wanted to shrink it, but I couldn't figure out a way. With this PR, I could reduce the number of lines to about 25% of the original length: https://github.com/qupath/qupath-extension-thresholding/pull/18#pullrequestreview-3479039144

I'm not convinced the PR is the right way to handle things, but it's such a core method any time we're using imglib2 that's worth trying to figure out how to make it as to call as possible - trying to minimise the need for duplication, generic gymnastics and unchecked exceptions elsewhere.

[Rylern]

Is that any better at runtime though?

The ClassCastException also happens at runtime no?

How would the user know which type to provide, unless they are essentially replicating every time what getTypeOfServer() is doing to check the PixelType?

They have two choices:

• Use the unsafe function, in which case the code is very short but may throw ClassCastException.
• Replicate what getTypeOfServer() is doing, in which case the code is long but won't throw ClassCastException.

That's ok for an example, but won't help us much in practice. In general we won't know what the type is for an image that hasn't been opened.

This was just to indicate that if you want to avoid ClassCastException and Unchecked cast warnings, you have to provide the type when creating the image.

My last suggestion was to provide both options: where a type is provided, and where either isn't

OK, but then if you want to avoid ClassCastException in createForLevel(int level, T type), you need to need to check that the provided type matches with the PixelType of the provided ImageServer, which means the deleted checkType() function should come back, as well as the long Javadoc explaining what types correspond to what PixelType.

Do you already have ideas about additional parameters to add? If not, I think static methods would simplify use considerably. And we can always add a builder later if it becomes necessary.

No, we can use static methods.

I think I'd need an example to understand what exactly isn't working.

On your PR, if I make getTypeOfServer public and add a createForLevel(int level, T type) function, the following code doesn't compile:

var imageServer = ...
var type = ImgCreator.getTypeOfServer(imageServer);
var image = ImgCreator.builder(imageServer).build().createForLevel(0, type);

because reason: no instance(s) of type variable(s) T exist so that ? extends NumericType<?> can be converted to T.

Would you use it differently?

My starting point was code related to thresholding that looked very repetitive. I wanted to shrink it, but I couldn't figure out a way. With this PR, I could reduce the number of lines to about 25% of the original length: qupath/qupath-extension-thresholding#18 (review)

You could shrink it by using the unsafe function. Here is an example:

private static ImgLib2AutoThreshold<?> createImgLib2AutoThreshold(ImageServer<BufferedImage> server, double downsample, DimensionReduction dimensionReduction) {
    if (server.isRGB()) {
        if (dimensionReduction.changeType()) {
            return new ImgLib2AutoThreshold<>(Converters.convert(
                    ImgCreator.builder(server, new ARGBType()).build().createForDownsample(downsample),
                    new RgbaReductionConverter<>(dimensionReduction),
                    new DoubleType()
            ));
        } else {
            return new ImgLib2AutoThreshold<>(Converters.convert(
                    ImgCreator.builder(server, new ARGBType()).build().createForDownsample(downsample),
                    new RgbaReductionConverter<>(dimensionReduction),
                    new UnsignedByteType()
            ));
        }
    } else {
        return createRealTypeImgLib2AutoThreshold(server, downsample, dimensionReduction);
    }
}

private static <T extends RealType<T> & NativeType<T>> ImgLib2AutoThreshold<?> createRealTypeImgLib2AutoThreshold(ImageServer<BufferedImage> server, double downsample, DimensionReduction dimensionReduction) {
    RandomAccessibleInterval<T> accessible = (RandomAccessibleInterval<T>) ImgCreator.builder(server)
            .build()
            .createForDownsample(downsample);
    if (dimensionReduction.changeType()) {
        return new ImgLib2AutoThreshold<>(convertImage(accessible, accessible.getType().copy(), dimensionReduction));
    } else {
        return new ImgLib2AutoThreshold<>(convertImage(accessible, new DoubleType(), dimensionReduction));
    }
}

This is quite similar with what you suggested on https://github.com/qupath/qupath-extension-thresholding/pull/18#pullrequestreview-3479039144.

[Rylern]

To recap, because I think this is getting complicated:

• Many methods don't/shouldn't care that we have a NativeType, but rather only that we have a NumericType: yes, we should only specify NumericType.
• Using a builder vs using static factory functions: yes, we can use static factory functions.
• Determining the type when creating an instance of ImgCreator vs when calling createForLevel() or equivalent: I think it's better to do it when creating an instance of ImgCreator, for reasons mentioned in second paragraph of Simplify ImgCreator by reducing use of NativeType and generic parameters #14 (comment)
• Should we have unsafe or safe ways to create images, or both:
  • Not providing the image type makes the function unsafe (i.e. can throw ClassCastExceptions and Unchecked cast warnings), but the calling code is much shorter.
  • Providing the image type doesn't throw exceptions, but the calling code is long.
  • We can have both or only the unsafe way. I would prefer both as it gives more flexibility.

[petebankhead]

Still thinking, so only a quick answer: instances of ImgCreator aren't doing much except retaining the ImageServer and the cache. It is unlikely that an ImgCreator would be retained and used later in the code.

Perhaps ImgCreator and the builder could be merged into one, e.g. something like this

List<Img<ByteType>> images = ImgCreator.builder(server)
             .type(new ByteType()) // Optional, for safety
             .cache(cache)              // Optional
             .buildForAllLevels()     // Different build options according to what the user wants

RandomAccessibleInterval<?>> intervals = ImgCreator.builder(server)
             .buildForDownsample(2.5)