What bool value should `imageRegion.IsInside(zeroSizedRegion)` return?

Algorithms
1

There appears a disagreement between the documentation of ImageRegion::IsInside(const Self &) and the actual behavior when the region passed as argument has zero as size value.

ITK: itk::ImageRegion< VImageDimension > Class Template Reference says:

template<unsigned int VImageDimension>
bool itk::ImageRegion< VImageDimension >::IsInside( const Self &  region ) const

Test if a region (the argument) is completely inside of this region. If the region that is passed as argument to this method, has a size of value zero, then it will not be considered to be inside of the current region, even its starting index is inside.

However, it appears that currentRegion.IsInside(zeroSizedRegion)) returns true when the index of such a ā€œzero sized regionā€ (zeroSizedRegion.m_Index) is inside the current region, specifically when this index is not at the left or upper border of the current region!

I hope you all agree that either the code or its documentation (or both) should be fixed. So I submitted two proposals, which are mutually exclusive:

Fix the codeā€¦

ā€¦ or fix the documentation (also proposing a code improvement):

What do you think? Does it matter at all, I mean would you possibly pass a zero-sized region as argument to ImageRegion::IsInside. If so, what Boolean return value would you expect?

Note that both (mutually exclusive) pull requests propose a code adjustment that is likely to improve the performance of ImageRegion::IsInside :smiley:

1 Like
2

I havenā€™t ever used ThisRegion.IsInside(QueryRegion) so I donā€™t know the typical use case. I imagine that it is used to check whether iterating over the voxels of QueryRegion will give voxels that are guaranteed to be inside ThisRegion; the processing of each voxel can assume inside-ness without having to check each one. If this is the use case then returning true for a QueryRegion of zero voxels seems right; no checks for inside-ness will be required.

2 Likes
3

I agree with Leeā€™s assessment. Thinking of IsInside as a subset:

From set theory: The empty set is a subset of any other set , but not necessarily an element of it.

But I am OK with either fix.

4

Well, I do understand the comparison to a set. But a zero-sized region isnā€™t just like a set to me. It still has an index (the start corner position). So I do think itā€™s reasonable to expect that a zero-sized region is considered inside an outer region, when its index is inside the outer region.

But I am also OK with either fix. :slight_smile:

5

OK. I guess that is that a zero-sized region with an index represents a point. I think that interpretation leads to.

ā€œa zero sized region, with index inside ThisRegionā€ returns true.

2 Likes
6

For the record, the pull request that ensures that region.IsInside(zeroSizedRegion) returns false for any zero-sized region as argument is now preferred over the other pull request, at least by @Lee_Newberg and @dzenanz. And itā€™s also fine by me :smiley: This pull request implements the behavior, specified by Luis Ibanez back in 2009, and still conforms with the current documentation

7

And that PR has now been merged.

1 Like
8

Thanks @dzenanz

BTW, We could still place the old (inconsistent and unnecessarily slow) implementation within an #ifndef ITK_LEGACY_REMOVE, if some user might really want to have it. But maybe it isnā€™t worth the effort. :thinking: What do you think?

9

I doubt anyone relied on it, given its inconsistency and being the opposite of documentation.

1 Like
10

Maybe we should update remote modules to use this version? Then we will have a much better sample.