Streaming for itkResampleImageFilter in case of linear transforms

[romangrothausmann]

This PR corresponds to #82, rebased onto master, meant for inclusion into master (ITK-5 instead of ITK-4.13.1).
Inspired by the discussion of the discourse topic Why ResampleImageFilter is slow?, this PR is a try to realize streaming for at least some transforms for the itkResampleImageFilter.
It is one of the most general filters in ITK and much used and the essential one for isotropic down-sampling of large datasets. Especially for that case it would be very helpful to have streaming capabilities, in order to generate smaller datasets for filters that cannot stream. Apart from a reduced memory footprint, streaming can lead to a significant speed up because a chunk can be processed while the next is loaded from disc, drastically reducing loading latencies.

As the filter is of great importance, some new tests cases (failing with the current implementation) were created (#392, now merged) that are expected to succeed (partially) when streaming is realized. With these tests (and possibly #468) master is equipped for testing the new contributions to itkResampleImageFilter, which enable streaming for linear transforms. Manual test showed that streaming already works but (even for an identity transform) the result's extent is larger than without streaming.

Therefore, itkResampleImageTest7 is meant to compare the current results of itkResampleImageFilter in various cases with those obtained when streaming is employed. There, a reader and a writer are avoided to rule out any interference concerning streaming limitations in respect to a chosen file format.
itkResampleImageTest2s is based on itkResampleImageTest2 adjusted to write MHAs, which support streaming.

[romangrothausmann]

The individual commits are in https://github.com/romangrothausmann/ITK/commits/streaming4ResampleImageFilter_incr, in particular with romangrothausmann@2cdc0ee the streaming tests are now expected to pass, which they do not (https://circleci.com/gh/InsightSoftwareConsortium/ITK/3182?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link):

In ResampleImageTest7 the monitor filter reports:
Down stream filter didn't execute PropagateRequestedRegion well
and
ResampleImageTest2Streaming
reports:
Description: Requested region is (at least partially) outside the largest possible region.
which could hint towards the problem I was experiencing before (https://discourse.itk.org/t/why-resampleimagefilter-is-slow/1217/17?u=grothausmann.roman), i.e. that the output extent is different with streaming.

It is unclear to me where this problem comes from and would need some help on this. @thewtex @blowekamp any idea?

[dzenanz]

Looks good after a glance. Someone else should look too!

[romangrothausmann]

Many thanks @dzenanz for taking a look. I would say this should not be merged until the extent problem is solved and the tests pass as expected.

[thewtex]

@romangrothausmann a few comments inline.

[romangrothausmann]

Many thanks @thewtex for taking the close look. Applied your suggestions (which all make sense) but the tests still fail for me locally with the same errors.

[romangrothausmann]

The test also fail on CI. @thewtex any ideas would could be wrong with the current SDI implementation?

[thewtex]

@romangrothausmann I'll dig in... 🏊‍♂️

[thewtex]

Tests are passing now 🎉 :-)

There were a few tricky items:

point = transformPtr->TransformPoint(currentCornerIndex);

should be

point = transformPtr->TransformPoint(point);

2. We needed to handle the cases

• Input requested region is completely outside the largest possible region.
• Input requested region is partially outside the largest possible region.
• Input requested region is completely inside the largest possible region.
• Input requested region completely surrounds the largest possible region.

3. There were a number of details in the tests that needed to be fixed to prevent false positive failures.

Items addressed

[romangrothausmann]

Wow, many thanks @thewtex. I'm trying to understand your changes.
With some rebasing magic I think I managed to create a commit that should represent your changes on top (romangrothausmann@29db4ad), at least it looks similar to the first force-push view.
Is there any easy way to "pull" that view into the local git such that I would get the same view in gitk that viewing romangrothausmann@29db4ad gives me?

[romangrothausmann]

point = transformPtr->TransformPoint(point);

True, many thanks for catching that, otherwise romangrothausmann@29db4ad#diff-43a4309c31e92fe375554af9562c5a04R221 makes no sense.

So

Input requested region is completely inside the largest possible region.

is covered by:
romangrothausmann@29db4ad#diff-a446710011999e954fe6b67b68d64bebR546

Not sure about
romangrothausmann@29db4ad#diff-a446710011999e954fe6b67b68d64bebR542
To my understanding it tests if "Input requested region" partially overlaps with "largest possible region".

[thewtex]

Is there any easy way to "pull" that view into the local git such that I would get the same view in gitk that viewing romangrothausmann/ITK@29db4ad gives me?

It is possible to pull and create a new branch, then do the diff. git series is also another tool for this.

So

Input requested region is completely inside the largest possible region.

is covered by:
romangrothausmann@29db4ad#diff-a446710011999e954fe6b67b68d64bebR546

Not sure about
romangrothausmann@29db4ad#diff-a446710011999e954fe6b67b68d64bebR542
To my understanding it tests if "Input requested region" partially overlaps with "largest possible region".

I will add some comments to the code...

[dzenanz]

Is there any easy way to "pull" that view into the local git such that I would get the same view in gitk that viewing romangrothausmann/ITK@29db4ad gives me?

You could try using git cherry-pick:

git checkout 29db4ad
git cherry-pick edd815b

These should give you only the relevant changes from Matt's commit.

[romangrothausmann]

How do you know "that last sampled chunk is completely outside the input"?

Is that the reason to remove itkPipelineMonitorImageFilter in itkResampleImageTest7.cxx (https://github.com/InsightSoftwareConsortium/ITK/compare/d4e1177f61ba9414f1ae05852384e5ec4e725c31..b1ee7d74b5fc198ea61c4cd6d54d18a14ad58b4c#diff-dfa5983cf67253ce0e03aca56a207db2L23)?

[romangrothausmann]

Thanks @thewtex, @dzenanz for your feedback.
The git cherry-pick lead to quite a few merge conflicts, so in the end I rebased my streaming4ResampleImageFilter_incr onto the same parent as edd815b and did:

git checkout  origin/streaming4ResampleImageFilter_master -- `git diff-tree --no-commit-id --name-only -r origin/streaming4ResampleImageFilter_master`

Have to check git series. Something new every day...

[thewtex]

How do you know "that last sampled chunk is completely outside the input"?

I spent a bit of time debugging the test :-).

Is that the reason to remove itkPipelineMonitorImageFilter in itkResampleImageTest7.cxx (https://github.com/InsightSoftwareConsortium/ITK/compare/d4e1177f61ba9414f1ae05852384e5ec4e725c31..b1ee7d74b5fc198ea61c4cd6d54d18a14ad58b4c#diff-dfa5983cf67253ce0e03aca56a207db2L23)?

It needed to be removed because the PipelineMonitorImageFilter did not behave correctly with a generated image as an input versus a reader or another filter as the input on multiple runs -- the buffered region was being reset to zeros. Since we already covered it in ResampleImageTest2Streaming, I removed it in ResampleImageTest7.

[romangrothausmann]

It needed to be removed because the PipelineMonitorImageFilter did not behave correctly with a generated image as an input versus a reader or another filter as the input on multiple runs -- the buffered region was being reset to zeros. Since we already covered it in ResampleImageTest2Streaming, I removed it in ResampleImageTest7.

Hm, I see, but shouldn't we include some means to check that the streaming actually happens? In a case where the streaming does not happen (though requested) the test would always succeed.
On the other hand I though it would be good to avoid a reader in this test (such that it is not dependent on the input file format) and just to check that streaming an no-streaming leads to the same result.

[thewtex]

Hm, I see, but shouldn't we include some means to check that the streaming actually happens? In a case where the streaming does not happen (though requested) the test would always succeed.
On the other hand I though it would be good to avoid a reader in this test (such that it is not dependent on the input file format) and just to check that streaming an no-streaming leads to the same result.

Good point. I will add a check at the end of streaming to verify that we are setting the requested region to a smaller chunk when streaming.

[thewtex]

Next patch set adds the streaming verification to itkResampleImageTest2Streaming and fixes the failing test by removing the Superclass::GenerateInputRequestRegion(); call -- this copies the output requested region to the input, but we do not want that behavior in this case.

[romangrothausmann]

Way cool! Many, many thanks @thewtex for catching the problem with Superclass::GenerateInputRequestRegion();, all your help and merging.

A comment of yours

ITK/Modules/Filtering/ImageGrid/test/itkResampleImageTest2Streaming.cxx

Lines 158 to 160 in 6093197

	// Note: We will only request the input 4 times because that last sampled
	// chunk is completely outside the input and not requested
	if( !monitor->VerifyInputFilterExecutedStreaming(4) )

makes me wonder if I do not understand the pipeline execution correctly.
Aren't we asking for 8 divisions in:

ITK/Modules/Filtering/ImageGrid/test/itkResampleImageTest2Streaming.cxx

Line 154 in 6093197

writer1->SetNumberOfStreamDivisions(8); //split into 8 pieces for streaming.

which according the the docs (https://itk.org/Doxygen/html/classitk_1_1PipelineMonitorImageFilter.html#a1e98d85c0f5a7ca8814a9c0d9d0d59e4) would have to match those checked by VerifyInputFilterExecutedStreaming as long as the argument is not given as a negative number?
Your comment however sounds like the monitor-filter is doing the requesting not the writer. Does monitor->VerifyInputFilterExecutedStreaming(4) override writer1->SetNumberOfStreamDivisions(8)?

How did you come up with the values in these checks:

ITK/Modules/Filtering/ImageGrid/test/itkResampleImageTest7.cxx

Lines 134 to 137 in 6093197

	TEST_SET_GET_VALUE( 0, finalRequestedRegion.GetIndex(0) );
	TEST_SET_GET_VALUE( 49, finalRequestedRegion.GetIndex(1) );
	TEST_SET_GET_VALUE( 59, finalRequestedRegion.GetSize(0) );
	TEST_SET_GET_VALUE( 10, finalRequestedRegion.GetSize(1) );

I assume they are dependent on the input image extents and the chosen splitter (ImageRegionSplitterMultidimensional for the used StreamingImageFilter), are they?

[thewtex]

Your comment however sounds like the monitor-filter is doing the requesting not the writer. Does monitor->VerifyInputFilterExecutedStreaming(4) override writer1->SetNumberOfStreamDivisions(8)?

Yes, this was a bug -- fixed here :-): #654

How did you come up with the values in these checks:

This is the final request region chunk -- yes, indeed, this is determined by the logic in StreamingImageFilter and the data size.