Add converters to and from ImgLib2 types

imagejan commented 5 years ago

This PR resolves #80 (except for the changes in result post-processing which are tracked in https://github.com/scijava/scijava-table/issues/5).

Remaining tasks:

[ ] the NumberToNumericTypeConverters by Leon Yang were just copied over from imagej-server; should we consider migrating their git history as well, @ctrueden ?
[ ] implement more converters in both directions, i.e. for (Unsigned)ShortType, BitType, etc.
[ ] the license headers should be added/updated

imagejan commented 5 years ago

A question to the Java generics experts (likely @ctrueden):

Is it possible to implement a Converter<Integer, IntegerType<T>> that can convert to sub-classes of NumericType? Such that when you call e.g. convert(someInteger, UnsignedByteType.class), it would generate a new UnsignedByteType(someInteger), and when you call convert(someInteger, ShortType.class), it would create new ShortType(someInteger), etc. for all implemented sub-classes of IntegerType?

Or do we need to implement an explicit converter for every ImgLib2 *Type as present for four of them currently in imagej-server (Double, Float, Integer, Long)?

I figure that for the other direction it should be simple, as long as we convert(IntegerType, Integer) with the help of IntegerType.getInteger(). So we probably need:

IntegerTypeToIntegerConverter using getInteger()
BooleanTypeToBooleanConverter using get()
RealTypeToDoubleConverter using ComplexType#getRealDouble()
maybe RealTypeToFloatConverter using ComplexType#getRealFloat() ?

What do you think?

tpietzsch commented 5 years ago

Is it possible to implement a Converter<Integer, IntegerType<T>> that can convert to sub-classes of NumericType? Such that when you call e.g. convert(someInteger, UnsignedByteType.class), it would generate a new UnsignedByteType(someInteger), and when you call convert(someInteger, ShortType.class), it would create new ShortType(someInteger), etc. for all implemented sub-classes of IntegerType?

This is not possible in general if you only have the Class. If you had an instance of the concrete Type you could call its createVariable() method to make a new one. If you know the Class has a no-argument constructor that does what you need, you can use reflection to get and call that. For example, this works:

public static void main( String[] args )
    throws NoSuchMethodException, IllegalAccessException, InvocationTargetException, InstantiationException
{
    UnsignedByteType t = g( UnsignedByteType.class );
    System.out.println( "t = " + t );
}

public static < T extends RealType< T > > T g( Class< T > klass )
    throws NoSuchMethodException, IllegalAccessException, InvocationTargetException, InstantiationException
{
    T t = klass.getConstructor().newInstance();
    t.setReal( 2 );
    return t;
}

I don't know about the performance penalty though. That should be tested...

imagejan commented 4 years ago

While writing a test for an unrelated task, I discovered that org.scijava.convert.DefaultConverter will handle conversion for:

Objects where the destination Class has a constructor which takes that Object

... which is true for most of (all?) the Number to *Type conversions.

As such, the converters (written by @lnyng) that I wanted to migrate from imagej-server to here (in https://github.com/imagej/imagej-common/pull/82/commits/37fd588863d185209430503fa96b54cba4107c1d) are actually completely unnecessary (!).

The other direction (NumericType -> Number) remains valid and necessary.

I'll try to update this pull request soon (keeping the tests for conversion in both directions, and adding just the necessary converters).

ctrueden commented 4 years ago

@imagejan The eventual goal was to split up the DefaultConverter into pieces. And maybe consider whether that auto-wrapping constructor magic is actually wise... but since that's how things currently work, and we shouldn't break backwards compatibility by changing it, your plan (of not implementing the unnecessary conversion directions) sounds good to me! :+1:

gselzer commented 2 years ago

@imagejan I cleaned up your work in an effort to make progress on https://github.com/imagej/pyimagej/issues/162.

Do you feel that it is necessary to keep these five tests? I guess they are special cases...

@ctrueden I rebased this over the latest master, and we have all passing tests. Can we get this merged quickly?

I'm not quite sure that the files I edited are correctly formatted, got to remember how I had set up formatting before in IntelliJ. I think I'll have to save that for tomorrow...

gselzer commented 2 years ago

I actually think I got the formatting setup correctly in 6a7f8f4. Does this commit's formatting look right to you @ctrueden?

imagejan commented 2 years ago

Thanks @gselzer for updating this PR!

Do you feel that it is necessary to keep these five tests? I guess they are special cases...

It's not exactly necessary, but they don't hurt, right? Just some (common) special cases to make sure they work.

gselzer commented 2 years ago

It's not exactly necessary, but they don't hurt, right? Just some (common) special cases to make sure they work.

Might as well keep them then, they don't hurt to keep.

ctrueden commented 2 years ago

Thank you @imagejan for this work, and thank you @gselzer for spurring it forward.

imagej / imagej-common

Add converters to and from ImgLib2 types #82