Infinite Loop when Sampling with Empty Region

[awalter17]

When a SamplingIterableInterval is created with an empty mask/region, it is possible to get stuck in infinite loops such as this one.

This is because the Cursor will only return a value if the mask/region is true at that position. For empty mask/regions this will never occur. See example below:

Img< BitType > mask = ArrayImgs.bits( 10, 10 );
Img< ARGBType > input = ArrayImgs.argbs( 10, 10 );
IterableInterval< ARGBType > sample = Regions.sample( Regions.iterable( mask ), input );
sample.firstElement(); // infinite loop

[awalter17]

I’m wondering how to best fix this. Probably we should just throw NoSuchElementException when firstElement()is called on an empty IterableInterval.

It’s also not ideal (to say the least) that fwd() can get caught in a infinite loop. However, I’m hesitant to just put a if(hasNext()) in there. I would do some benchmarks first to see how big the impact is…

Quoting @tpietzsch - copied from this forum discussion

[awalter17]

Good news, I don't think hasNext() is necessary here.

When an IterableRandomAccessibleRegion is created it counts the number of trues in the mask (line 65). This propagates and eventually becomes maxIndex in RandomAccessibleRegionCursor. So, in fwd() we just need to add the following line:

if (maxIndex == 0)
    throw new NoSuchElementException();

That being said, should we even allow empty masks to become IterableIntervals? Or should we throw an IllegalArgumentException sooner? This could be done by checking if Regions.countTrue( ... ) is zero, and if at some point we switch this to use MaskInterval we could also call isEmpty() on that.

Thoughts @tpietzsch?

[tpietzsch]

We should definitely add

@Override
public Void firstElement()
{
    if ( size() == 0 )
        throw new NoSuchElementException();
    return cursor().next();
}

to https://github.com/imglib/imglib2-roi/blob/0b560b3ee327455741cc5a4c9c4c3cfbd8e91b7a/src/main/java/net/imglib2/roi/util/IterableRandomAccessibleRegion.java#L80-L84

That being said, should we even allow empty masks to become IterableIntervals? Or should we throw an IllegalArgumentException sooner? This could be done by checking if Regions.countTrue( ... ) is zero, and if at some point we switch this to use MaskInterval we could also call isEmpty() on that.

I would allow it.

So, in fwd() we just need to add the following line:

if (maxIndex == 0) throw new NoSuchElementException();

I would also benchmark this before adding. It might have overhead for the "normal" case. Also I would not throw NoSuchElementException here. Just do nothing and return. Calling fwd() when hasNext()==false is undefined behaviour. But I would not throw an exception before trying to get() the invalid value. That is consistent with how RandomAccesses work everywhere. You can move them around all you want, OutOfBoundsException only occurs when you actually try to get() the invalid value.