Add utility class to wrap images as 1D collections

[ctrueden]

This is useful to pass images to APIs that work with collections, such as Guava's com.google.common.math.Quantiles.

See also this post on the Image.sc Forum.

[ctrueden]

A couple of notes.

1. I just noticed @acardona wrote a similar thing on the collection branch. But it doesn't look like a PR was ever filed. Also, that work always maps IterableInterval<T> and RandomAccessibleInterval<T> to the same type Collection<T>, without a way to bake in a conversion function. But if this PR merges, and @acardona is happy with it, then we should probably also delete the collection branch, since it would not be necessary anymore.

2. There is a subtle (to me 😉) bug in the current implementation, relating to images with non-flat iteration order. You can see the problem by adding the following code to the beginning of the testDoubleList() method:

   final Img< DoubleType > imgDouble = new CellImgFactory<>( new DoubleType() ).create( 30, 40, 50 );
   double v = 0; for ( final DoubleType t : imgDouble ) t.set( v++ );

   The issue is that this code uses IntervalIndexer to map between 1D and nD coordinates, but that function uses flat iteration order. So for things like CellImg, the original image iteration order (not flat) does not match the 1D rasterization order of the wrapped collection (flat). Is there an efficient way to introspect a CellImg to discern its 1D-to-nD ordering? If not, I am not sure how to solve this problem. Do we feel that the natural iteration order of the Img necessarily needs to match the natural iteration order of its wrapped List?

[maarzt]

I like the idea of this commit. Enabling the conversions:

1. InterableInterval -> Collection
2. RandomAccessibleInterval -> List

is nice. But I don't like that we need to add so many methods. We would need to add even more methods for each new type, and collection class supported. I would definitely drop the support for boxed Byte and Short.
One could avoid having so many methods, be using some builder pattern. Or providing a type converter as parameter just like Converters.convert(image, converter, type).

Another thougth: This doesn't seem to be so much imglib2-core to me. The functionality is comparable to ImageJFunctions.wrap(...) in imglib2-ij. I personally would consider adding it to algorithms.

[maarzt]

1. I think it would be nice to use image as a parameter name rather than ii.
2. The method would benefit from a javadoc.

[maarzt]

1. First parameter must not be named ii. Use image instead.
2. Javadoc would be nice

[maarzt]

1. Name size32 doesn't make much sense. sizeAsInt would be better.

[maarzt]

This method is not necessary. It's performance and memory consumption is probably the same as integerCollection. And ByteType is also not very widely used.

[maarzt]

Not needed integerCollection could be used instead

[maarzt]

Remove method. integerList can be used instead.

[maarzt]

Remove method. integerList can be used instead.

[maarzt]

I would not test that many values. Even testing one value per type should be enough.

[ctrueden]

There is no performance issue with these tests. Testing more edge cases is good. I see no benefit in cutting down these tests.

[maarzt]

I also see no performance issue. My points are these:

1. The tests could be simpler. A simple test, makes maintenance easier. When it fails, finding the bug is easier.
2. The test implement the same algorithm as the tested code. If there's a bug with for example the usage of IntervalIndexer, the test want show that, because it would compare the wrong result against an equally wrong expected result. So having a test, which is simple enough to not use for example IntervalIndexer is actually a benefit.
3. There are better places to put test getRealDouble and similar methods. See PR Simple Conversion between RealTypes #249 fixes a bug in Unsigned128BitInteger.getRealDouble and also adds tests.

[maarzt]

On the other side. The tests can stay as the are they are good enough.

[maarzt]

Could be instead:

List<String> pixels = Array.asList("A", "B","C","D");
Img<String> img = new ListImg(pixels, 2, 2);
List<String> result = FlatCollections.collection(img, s -> s);
assertCollectionsEquals(pixels, result);

[maarzt]

Could be simpler, if we test only for one value:

assertCollectionEquals( Arrays.asList(42), FloatCollections.integerCollection(ArrayImgs.bytes(byte[]{42},1)) );

[ctrueden]

Thanks for the review, @maarzt!

I force-pushed an update with the following improvements:

1. I added javadoc to all public methods.
2. I renamed the ii parameters to image.

I did not (yet) change either of the following:

• Request to remove the byte and short signatures.
• Request to make the testing more minimal.

I think it is a mistake to cut down the testing. Fewer edge cases would be covered. The tests as written are simple to understand and not burdensome to maintain.

Regarding removal of the byte and short signatures: generally speaking, I agree with you that less code is better. Every line of code is a liability, as they say. However, in this case, there are 8 primitive types, and each of them (except char) corresponds well to a particular subset of the ImgLib2 type hierarchy, as written here. In my opinion, removing byte and short here would make only make this code more confusing to newcomers, who would see that most but not all of the primitive types have corresponding signatures, and wonder why not. These methods are a convenience, only one line each, so near zero burden to maintain. But replicating them via the general method is potentially error-prone, because each one-line conversion embeds the correct method call (e.g. t.get() vs. .getInteger() depending on the type) to access the underlying data most directly.

Finally, I disagree that this utility class would be better served in imglib2-algorithm, for the following reasons:

• Other conversion methods such as Converters and Views exist in imglib2 core, not imglib2-algorithm.
• The imglib2-algorithm library brings in many dependencies, none of which are necessary for FlatCollections.

If it is decided that this utility class is too "bloaty" for ImgLib2 core, I'd favor just putting it into ImageJ Ops instead. But I think it would be a shame to reduce its visibility that way.

[maarzt]

After thinking about it a little bit more. I agree that this PR is mostly fine as it is. Tests are ok, having all the methods is ok, having it as part of imglib2 is fine for me too. But I still have some worries:

• About Byte and Short. I see how supporting byte and short is more inviting to newcomers. At the same time I'm worried, because we only support the rarely used signed ByteType and not the much more common UnsignedByteType. So supporting ByteType but not the UnsignedByteType seems to be even more confusing than only having IntegerType???
• More javadoc is needed: The javadoc needs to state that these methods have low performance, because of there bad memory footprint related to the boxed primitives. And that the collection don't guaranty any particular iteration order.

[ctrueden]

I understand what you are saying about the tests. The way they are written, they are regression tests. They aim to be a little more thorough (exercising multiple methods of Collection and List), at the expense of layers of indirection with the helper methods and reuse of images. If you want to rewrite them to your liking, go for it. I won't be offended. But I don't have time personally.

I pushed two more commits: one removing the byte and short wrappers, and one adding a note to the javadoc about the iteration order.

Regarding performance of the wrapped collections, there should be virtually no memory overhead. Yes, there is boxing. However, it's not like ArrayList<Double> where there will be size() Double objects in existence simultaneously. If you really want me to benchmark it I will, but I do not expect the memory use to be too bad. Only time performance potentially impacted compared to primitives, as transiently creating all those boxed objects takes CPU time too.

[tpietzsch]

Looks ready to merge

[ctrueden]

Thanks @maarzt!