Reconsider ImgFactory generic parameter about imglib2 HOT 16 CLOSED

To explain a little: it is weird that ImgFactory takes a type T on the class, but you do not pass anything which anchors that to the constructor invocation. So any ImgFactory created via reflection at runtime, for example, does not even know its own type.

One question is: why does an ImgFactory need a type at all? If the type is simply passed to the create method calls, that should be sufficient, right? And actually it already is. If the type param were removed from the class itself, we wouldn't need the imgFactory method anymore for switching types.

Alternately, option 1 binds each ImgFactory instance to a specific type T, but then eliminates the need to pass an explicit type instance every time create is called. This avoids the chance of runtime "skew" between the intended type of the ImgFactory, and the actual type of the type instance passed to create.

Both options above are proposed so that backwards compatibility is maintained—i.e., so we don't have to bump to imglib2-3.0.0.

from imglib2.

@tpietzsch Any chance you are interested in a PR addressing this issue? I can file one, but would need to know which option above you feel is more appropriate. And of course, depending on the choice, it could break backwards compatibility, so might want to wait a while till we are ready for imglib2 5.0.0.

from imglib2.

@ctrueden Yes, I'm interested. I also find the need to specify the type twice annoying. In principle I would prefer option 2: removing the type from the factory. But I'm not sure whether that is possible because NativeImgFactory could not extend ImgFactory anymore. But I'm not sure and it might be worth trying.
@axtimwalde, can you comment on this please?

from imglib2.

@tpietzsch @ctrueden without much though I am all for it. The type belongs into the factory methods not into the factory. However, a type in the factory could provide bounds to the type methods are willing to accept which would enable to have NativeImgFactory extends ImgFactory. I tried that quickly and it basically breaks all of ImgLib2 and will not go easy without significant API overhaul. Better ideas?

from imglib2.

Doing anything like NativeImgFactory vs ImgFactory requires a type parameter on the factory, otherwise we cannot narrow applicability to only specific types. It's certainly a bigger project, but if we can find a nice solution it would be worth to break API. I have thought about it for a while and I do not see a perfect solution so far.

Related (but possibly orthogonal) things that we should do while we are at it:
1.) Make ImgFactory an interface.
2.) Split the part of NativeImgFactory that is consumed by NativeType.createSuitableNativeImg into a separate interface. There is no reason to mash these into the same inheritance hierarchy. (This way, if the user gets a NativeImgFactory (by interface) he does not see the strange createLongInstance, createFloatInstance, etc methods that make absolutely no sense in particular if the factory is typed.)

In principle there could be just a untyped ImgFactory interface with a createGeneric or tryCreate method that throws UnsupportedTypeException. More specific factories could add more concrete create methods that have bounds on the method parameters. I already have such a create method in ReadOnlyCachedCellImgFactory (which does not extend ImgFactory which allows me to do this...)

from imglib2.

How about this? https://github.com/imglib/imglib2/compare/img-factory

It implements Option 1 above, which is much less destabilizing to the rest of the ImgLib2 codebase and downstream components. It was slightly more complex than stated above due to the introduction of the Supplier< T > option since this issue was written, but actually it's great because now the supplier is handled uniformly across all ImgFactory implementations.

This change also improves the type safety and elegance of the imgFactory( S type ) methods.

If you guys like it, I can file it as a PR.

@tpietzsch Regarding your recent comments above: I like what you are saying, but as you point out, those changes would be breaking. Should we merge my work here, and then leave this issue open as a reminder that we ultimately want to purge the generic property from ImgFactory and make the other changes you describe?

from imglib2.

@ctrueden Hmm, it is better than nothing, but is doesn't solve the problem in my opinion. I would rather try to come up with a better solution for a little bit longer, instead of making this a PR now and breaking API twice.

At the moment at least we are able to do

final ImgFactory factory = new ArrayImgFactory();
final Img< ByteType > bytes = factory.create( new int[] { 10, 10, 10 }, new ByteType() );
final Img< ShortType > shorts = factory.create( new int[] { 10, 10, 10 }, new ShortType() );

if we are not being nice and using raw types.
I think that, this is what we would actually want (but if possible more type-safe). We want to reuse the same factory to instead of creating a new factory for every new image. Ideally ArrayImgFactory would be a singleton.

from imglib2.

@ctrueden I thought more about it: Your branch would break existing code. All existing code creates ArrayImgFactory etc without a "factory type" parameter. So all create calls will throw NPE. This could be fixed by redirecting the new create-without-type calls to the deprecated ones, instead of the other way around. Even then, it is only safe to use the new create-without-type once we made sure factories everywhere are constructed with the constructor-with-type. There is no gradual migration path. Let's face it: this requires a breaking change.

from imglib2.

So all create calls will throw NPE.

No, only the new ones that do not pass the type. For existing code, everything should still work. The only thing that would break are third party classes extending ImgFactory and friends, I think.

from imglib2.

No, only the new ones that do not pass the type. For existing code, everything should still work. The only thing that would break are third party classes extending ImgFactory and friends, I think.

True, I saw that wrong.

Still the problem remains that we cannot use the create-without-type methods anywhere inside an algorithm for example because the factory passed in might be constructed with the old constructor-without-type.

In my opinion this is a mess and I would rather break API and fix code everywhere to set it right.

from imglib2.

@ctrueden @axtimwalde I also yesterday made several futile attempts to come up with an alternative solution. I think, @ctrueden solution is the only way that makes sense.

ImgFactory has to remain typed, otherwise we cannot restrict to NativeType in derived factories.
That means there has to be new factory instances for every new type we want to create.

These we get by

public abstract < S > ImgFactory< S > imgFactory( final S type ) throws IncompatibleTypeException;

It's infuriating that we cannot even derive this to return something more specific that ImgFactory, such as CellImgFactory<S> in CellImgFactory. But I guess everyone gets the type system they deserve ... :-)

In the end, instead of doing

factory.create( IntType::new, 10, 10, 10 );

it will be

factory.imgFactory( IntType::new ).create( 10, 10, 10 );

which is almost as convenient.
We might consider renaming imgFactory to forType

factory.forType( IntType::new ).create( 10, 10, 10 );

Still the problem remains that we cannot use the create-without-type methods anywhere inside an algorithm for example because the factory passed in might be constructed with the old constructor-without-type.

This could be fixed (almost) if the concrete factories would make new typed factory instances for the image instances they create.

public ArrayImg< T, ? > create( final long[] dim, final T type )
{
	return ( ArrayImg< T, ? > ) type.createSuitableNativeImg( this, dim );
}

would need to become

public ArrayImg< T, ? > create( final long[] dim, final T type )
{
	return ( ArrayImg< T, ? > ) type.createSuitableNativeImg( new ArrayImgFactory<>(type), dim );
}

etc.

And we need to remember to revert this once we remove the deprecated methods.

With this change I would be fine with making this into a PR, (...being finally convinced that this sets up the only sensible API 😢 )
@ctrueden If you are not annoyed too much by this already, please go ahead...

On a related topic: I would like to make IncompatibleTypeException into an unchecked exception (like IllegalArgumentException), because there are many cases where you just know that the exception cannot be thrown in the

factory.forType( IntType::new ).create( 10, 10, 10 );

and it is cumbersome to always write the empty try-catch boilerplate there.
Coincidentally, @maarzt came today with the same suggestion, but on a different problem. He will make an issue for this.

from imglib2.

@tpietzsch wrote:

This could be fixed (almost) if the concrete factories would make new typed factory instances for the image instances they create.

What if, instead of the ImgFactory having a final T t field, we removed the final modifier, and then if the t is still null, cache it when it is passed to one of the deprecated create methods? Then we could keep using this for createSuitableNativeImg.

In that way, any "bad" (no available type instance) ImgFactory becomes a "good" one as soon as any of the API methods are called that pass the instance.

from imglib2.

sounds good

from imglib2.

OK, working on it now!

from imglib2.

👍 thanks

from imglib2.

Resolved by #197. Thanks @tpietzsch!

from imglib2.