ops behaviour for RAI with non-zero interval offset

[tischi]

@imagejan @ctrueden

related to: https://forum.image.sc/t/measure-surface-perimeter-in-imglib2/21213

final RandomAccessibleInterval outputOutline = opService.morphology().outline( mask, true );

Above code crashes if Intervals.minAsLongArray( mask ) is non-zero, i.e. the interval has a non-zero offset.

I feel it is a strategic decision what to do here. My vote would be for all ops to respect non-zero offsets and "pass them on", i.e. the outputOutline should have the same interval offset as the input image.

Whatever the decision is, it is important to be consistent here, because this can lead to nasty hidden bugs if there is inconsistent behavior.

If we say that the offset must be zero, i.e., the inputs must actually beImg and not only RAI I would think there should be a crash with a message: Detected Non-Zero Offset: not allowed. Or something like this.

Applying Views.zeroMin( input ) implicitly at the beginning of each op would avoid such crashes, but I don't think it would be good, because it is not clear what's going on.

Happy to discuss this more!

[tischi]

For my own code I, e.g., wrote a function like below in order to be able to generate new RAI with the same offset as the old one:

https://github.com/tischi/fiji-plugin-morphometry/blob/master/src/main/java/de/embl/cba/morphometry/Utils.java#L733

public static < T extends RealType< T > & NativeType< T > >
	RandomAccessibleInterval< T > copyAsArrayImg( RandomAccessibleInterval< T > orig )
	{
		RandomAccessibleInterval< T > copy = new ArrayImgFactory( orig.randomAccess().get() ).create( orig );
		copy = Transforms.getWithAdjustedOrigin( orig, copy );
		LoopBuilder.setImages( copy, orig ).forEachPixel( ( c, o ) -> c.set( o ) );

		return copy;
}

[ctrueden]

I feel it is a strategic decision what to do here. My vote would be for all ops to respect non-zero offsets and "pass them on", i.e. the outputOutline should have the same interval offset as the input image.

I agree. @gselzer @MarcelWiedenmann When porting ops, we should test with non-zero offset. Ideally we'd do that for all relevant ops using common helper code for creating such RAIs, and then asserting the offsets remain correct.

[MarcelWiedenmann]

While I also agree generally, I also wonder what we should do in the case of Computer Ops where both the input and the output are provided by the user and their offsets differ. Should we align them? What if, for some reason, an Op considers pixel coordinates in its computation. Should we use the coordinates of the input? Should we just fail with a descriptive error in such cases? (Then again, this case may not be that important because the user has more control here.)

[tischi]

In general, the offsets may in fact differ, and the user might have a reason. For example, I quite commonly "paint" a smaller (input) image into a larger (output) image. For this use-case I would say we

• iterate over the input pixels
• set the result values in the output image at the coordinates of the input image

I feel this is a quite common imglib2 style pattern to do it like this:

inputCursor = input.cursor();
outputAccess = output.randomAccess();
while( inputCursor.hasNext() )
{
  inputCursor.next();
  outputAccess.setPosition( inputCursor );
  outputAccess.get().set( ... );
}

For me, this would follow the "principle of least astonishment" (in imglib2 world). And if above crashes, because something does not match then I would say it should loudly crash.

[EDIT]: However, I think there are also other places in imglib2, where the provided output image was used to get an iterator. The logic being that one could restrict computations to a certain part of the input image by specifying a smaller output image. Maybe, e.g., here: https://github.com/imglib/imglib2-algorithm/blob/master/src/main/java/net/imglib2/algorithm/convolution/Convolution.java#L26

Thus, the question is where to take the iterator from: input or output? Frankly, I don't know. Would it work to do: union = Intervals.union( input, output ) and perform computations on the union? I can't think of a case right now where that wouldn't work but probably I am missing something...

[MarcelWiedenmann]

My intuitive claim would be that obtaining the cursor from the output should work more often since many of the imglib2 algorithms I came across so far expect RandomAccessible inputs and RandomAccessibleInterval outputs (e.g., the convolution you mentioned), that is, obtaining a bounded cursor from the input would require some more work. My insight into the multitude of imglib2 algorithms is somewhat limited, however. Given we would agree on using the output's cursor, then input and output would need to be aligned, right? So if you would want to paint into a larger output image, you would need to Views#interval it first. And if you wanted to process a non-zero min input image, you would need to translate the output accordingly (e.g., if you would want to use a freshly created Img as output).

[tischi]

So if you would want to paint into a larger output image, you would need to Views#interval it first

That would be an option indeed. Thus maybe proposal:

• If an output image is provided take the interval from the output image.
• If there is no output image is provided, either operate in-place, or create an output image, based on the interval of the input image (that is: if the input image had a non-zero offset also the generated output image should have the same non-zero offset).

What do you think?

[ctrueden]

@tischi I think those are good policies, broadly speaking.

There are difficulties in certain cases:

• When plain Iterable (not IterableInterval) is used, there is no explicit interval available to use.
• When IterableInterval input is used, the algorithm is likely to loop the input, writing results into the output (in the case of a Computer op, at least). So if the output RandomAccessibleInterval has smaller bounds than the input IterableInterval, we would need to either: A) fail; or B) skip over input values outside the bounds of the output RAI. I'm fairly certain no ops currently take care to do (B); probably they fail somehow due to out-of-bounds issues.

Note that we have a general mechanism in place now ("op transformation") available for cases where an op implements a particular interface, but a different-but-compatible interface is desired. For example, if a Computer op exists, it can be coerced into a Function as long as the framework has a way to create the output needed by the computer. This creation mechanism can and should, in general, preserve the bounds of the input.

[gselzer]

I generally agree with what has been discussed.

@MarcelWiedenmann said:

Given we would agree on using the output's cursor, then input and output would need to be aligned, right? So if you would want to paint into a larger output image, you would need to Views#interval it first. And if you wanted to process a non-zero min input image, you would need to translate the output accordingly (e.g., if you would want to use a freshly created Img as output).

It seems to me that theoretically, the most consistent solution would be Intervals.union(input, output), as @tischi suggested. Let's say, for example that we want to do a Gaussian, with a 100x100 input image with the Interval min at (0, 0) and a 50x100 output image with the Interval min at (200, 200). What would we expect to happen? Should the Op fail? Should it align the minimums of the input and output? What if instead the min of the output is at (50, 0)? Then it seems most likely that we would want the output to be the gaussian of only the right half of the input image, especially since an output like that requires(?) deliberate transformation by the user.

The question here when there inputs have different offsets, I think, is how they were obtained and why they have different offsets. In the vast majority of cases we will not be dealing with images at different offsets; the user will most likely have called ops.create().img(input) and then passed that result as the output. If the offsets differ that usually (always?) requires the user to make a transform call (please correct me if I am wrong, I suppose maybe there are error cases where a user might want to reuse image objects for efficiency, offsets it for one Op then forgets to remove that offset before using in a second Op?); that will have to have some intent behind it. As such it seems that the theoretically most correct solution is Intervals.union, in which you only compute over the sections of the input that will fit into the output, although that might not be the best option in practice.

Edit: @MarcelWiedenmann pointed out to me that I had the wrong idea of what a union is. As such I would support Intervals.intersect(input, output) instead of Intervals.union(input, output) :grin:.

[tischi]

Sorry, yes: Intervals.intersect(input, output) would be the correct one! Thanks @MarcelWiedenmann for spotting that!