Skip to content

Conversation

maarzt
Copy link
Contributor

@maarzt maarzt commented Mar 5, 2019

When I need to convert between two RealTypes a usally us a lambda expression like this:

Converter<RealType, RealType> converter = (i, o) -> o.setReal(i.getRealDouble());

But this converter is not optimal in term of performance (To convert ByteType -> IntType you would not need do cast to double), a precision (UnsignedLongType can be poorly represented as double).

This PR adds three methods to fix this:

  • Getting converter between two RealTypes, which is optimal in terms of performance and precision, is as easy as:
Converter<UnsignedByteType, LongType> converter = 
     RealTypeConverter.getConverter(new UnsignedByteType(), new LongType());
  • Converting the type of a RandomAccessibleInterval:
RandomAccessibleInterval<LongType> converter = 
     RealTypeConverter.convert(image, new LongType());
  • Copy pixels between to RandomAccessibleIntervals:
CopyPixels.fromTo(imageA, imageB);

@maarzt
Copy link
Contributor Author

maarzt commented Mar 5, 2019

I'm not sure about the names of the static methods, or to which class the should belong: RealConverters.getConverter, RealConverters.convert and especially CopyPixels.fromTo.

And I think about adding more functionality: Converters that bound the value to minimum and maximum. And converters allow to scale the values.

@maarzt maarzt requested review from tpietzsch and axtimwalde and removed request for tpietzsch March 5, 2019 16:56
@tpietzsch
Copy link
Member

@StephanPreibisch @axtimwalde I just noticed (because this PR touches it), that Util.round() behaviour differs from Math.round() for negative numbers. E.g., Math.round() rounds -0.5 to 0, while Util.round() rounds -0.5 to -1.

Do you remember what the reason was to do it like this?
And if there was no specific reason, should we change it to just do Math.round()?

Copy link
Member

@tpietzsch tpietzsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the names of the static methods, or to which class the should belong: RealConverters.getConverter, RealConverters.convert and especially CopyPixels.fromTo.

I like RealTypeConverters.getConverter and RealTypeConverters.convert.

CopyPixels.fromTo is not good.
The name hides the fact that it converts between types.
Also, placed in a unrelated CopyPixels class, it is weird that it is limited to RealType.
I would like RealTypeConverters.copyConverted(...) better.

A general copy-pixels method would be nice though.
(There is ImgUtil.copy(...) but it's very specific. Maybe could be deprecated when something more general is available).
I would change signature to the source being only RandomAccessible (not Interval), so that it can be used to fill pixels of an output interval from a potentially unbounded source.

Then I could imagine to have a CopyPixels class with more methods where the converted copy would fit in. E.g.

public final class CopyPixels
{
	public static < T extends Type< T > > void copy(
			RandomAccessible< T > source,
			RandomAccessibleInterval< T > destination )
	{...}

	public static void copyReal(
			RandomAccessible< ? extends RealType< ? > > source,
			RandomAccessibleInterval< ? extends RealType< ? > > destination )
	{...}
}

And I think about adding more functionality: Converters that bound the value to minimum and maximum. And converters allow to scale the values.

I think this is a good idea, but could probably be done later in a separate PR.

}
}

public static class DoubleConverter implements Converter< RealType< ? >, RealType< ? > >
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these classes being public could you add javadoc, please.
Can be very minimal, e.g.,

/**
 * Converts via {@link RealType#getRealDouble}.
 */

is better than nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@tpietzsch
Copy link
Member

A general copy-pixels method would be nice though.
Also related: #237

maarzt added 3 commits March 14, 2019 15:30
The complicated code, that implements many different converters
and decides which one to use, is now nicely separated from the public
API.

The classes RealTypeConverters.DoubleType previously were public
to allow the use of ClassCopyProvider. No they are package-private
but ClassCopyProcider can still be applied.
@tpietzsch tpietzsch merged commit a34be5c into master Mar 14, 2019
@maarzt
Copy link
Contributor Author

maarzt commented Mar 14, 2019

@tpietzsch I did the changes you requested:

  • I moved the copy method to RealTypeConverters.copyFromTo(RandomAccessible source, RandomAccesibleInterval destination).
  • First argument of the copy method is now RandomAccessible.
  • I additionally moved all the private methods in RealTypeConverters to RealTypeConverterInternals. This is to better separate the public methods, from the many private methods. It also provides more space in the RealTypeConverters class, to add more functionality like scaling and bounded converters.

@tpietzsch
Copy link
Member

@maarzt I merged this accidentaly, now force-pushed to master to undo that.
So the state of this PR doesn't reflect reality unfortunately

@tpietzsch
Copy link
Member

I'll review and then merge locally...

@tpietzsch
Copy link
Member

ok, merged now...

@tpietzsch tpietzsch deleted the real-converters branch March 15, 2019 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants