new tests to verify streaming capabilities of itkResampleImageFilter by romangrothausmann · Pull Request #392 · InsightSoftwareConsortium/ITK

romangrothausmann · 2019-01-10T13:32:23Z

PR only for the tests that are expected to succeed with #82 (and to fail currently) in order to check their outcome without the changes to itkResampleImageFilter #82 (which currently appear to effect other tests like itkMultiResolutionPyramidImageFilterWithResampleFilterTest as well)
According to #84 (comment), history squashed into single ENH-commit rebase on to master.

creation history:

plain copy of itkResampleImageTest.cxx and its cmake
employ itkResampleImageTest2s, still the same as itkResampleImageTest2
write MHAs (which support streaming) instead of PNGs
demand streaming and employ where possible
verify streaming for linear case
itkResampleImageTest7.cxx for comparing output of stream driven imaging (SDI) and without SDI
mark resample as modified to enforce re-execution of filter chain

romangrothausmann · 2019-01-10T14:45:35Z

As some changes were done on the tests to conform to the ITK5 standards, I guess itkResampleImageTest2s.cxx and itkResampleImageTest7.cxx should be re-done to incorporate the changes introduced for ITK5, now that the PR is based on master instead of v4.13.1?

dzenanz

Yes, this should compile and test OK on master, and use ITKv5 style (using instead of typedef etc).

Modules/Filtering/ImageGrid/test/itkResampleImageTest7.cxx

romangrothausmann · 2019-01-23T13:03:46Z

Re-did the commit based on other the other test-files used now take from https://github.com/InsightSoftwareConsortium/ITK/tree/b2884129bf6db53cb102137d81e7176e893c5e5f
The corresponding branch that is not squashed is: https://github.com/romangrothausmann/ITK/commits/test_RSI-SDI_master_incr

romangrothausmann · 2019-01-23T14:19:27Z

New push contains changes as suggested by @jhlegarreta in #84 (review) together with a fixed typo.

romangrothausmann · 2019-01-23T15:14:10Z

It is not clear to me why the circleci job failed. If I'm not mistaken, both new tests passed according to the logs.

romangrothausmann · 2019-01-24T10:36:40Z

It is not clear to me why the circleci job failed. If I'm not mistaken, both new tests passed according to the logs.

For clarification: the 2 new tests should fail for current master, therefore I added the expectation to WILL_FAIL romangrothausmann@6b59a58 (with #82 the tests should succeed so with #82 the expectation should be changed then).

However, the tests seem to succeed currently, so they are not yet doing what they are meant to do. What could be missing? @blowekamp could you take a look?

romangrothausmann · 2019-01-24T11:01:37Z

Rebased onto current master @ 6218fca to rule out any problems due to updates since b288412
and added romangrothausmann@a7fbd1c to check that streaming fails in case of non-linear transforms. Still the test do not fail as currently expected.
itkResampleImageTest2s fails but not as expected:

Streamed pipeline was executed 8 times which was not the expected number 1 of times.
Streaming succeded for non-linear transform which should not be the case!

itkResampleImageTest7 succeeds which is not expected either:

Test with normal AffineTransform.
Trying streamer->Update()
Trying streamer->Update()
Test passed.

thewtex · 2019-01-24T15:25:03Z

Thanks for pushing this forward @romangrothausmann . I will take a look 👀

Modules/Filtering/ImageGrid/test/itkResampleImageTest7.cxx

thewtex · 2019-02-04T20:22:34Z

Taking a look, the PipelineMonitorImageFilter needs to be in a different position in the pipeline, i.e.,

reader -> monitor -> resample -> writerOrStreamingFilter

This way, if resample is streaming, it will only request smaller regions from monitor, and monitor can keep track of the regions it is requested.

In contrast, with this topology

reader -> resample -> monitor -> writerOrStreamingFilter

monitor does not know whether or not resample is streaming.

Have patch, will push :-)

…lter New tests are expected to fail as long as itkResampleFilter does not support streaming for linear transforms GH PR InsightSoftwareConsortium#82. Also, tests for comparing output with stream driven imaging (SDI) and without based on itkWarpImageFilterTest2.cxx and itkResampleImageTest.cxx.

These virtual methods are intended to be called internally by the ProcessObject and should not be publically accessible. Calling them may result in unexpected behaviour. Instead, call the relevant public methods provided by itk::ProcessObject.

thewtex · 2019-02-04T21:32:25Z

rebasing on master...

romangrothausmann · 2019-02-05T10:14:55Z

Cool. Many thanks @thewtex for taking a look and sorting things out. It wasn't easy for me to fully see your changes, i.e. those between my old 29f4e00 and the new one f973208 due to rebasing on newest master and squashing.
So I rebased 29f4e00 onto a1bd683 (parent of f973208) and renamed to itkResampleImageTest2Streaming.cxx accordingly which results in romangrothausmann@4566b74.
Sadly, GH comparison does not work well in that case (f973208...romangrothausmann:4566b7419b4c2bfa860a18957fd30db49eb26d55) showing itkResampleImageTest2Streaming.cxx and itkResampleImageTest7.cxx as new files instead of just the differences. So I create a new commit on top of romangrothausmann@4566b74 with the files as in f973208 resulting in romangrothausmann@e41fff8, so looking at that in GH should show the actual changes from @thewtex between 29f4e00 f973208 (please correct me if I got that wrong).

So there I can see that (apart from white space changes romangrothausmann@e41fff8#diff-50f53fc01f72ba938548d64a1218589bL79):

the unnecessary readers and their args got removed (romangrothausmann@e41fff8#diff-50f53fc01f72ba938548d64a1218589bL93).
typdefs replaced by using (sorry, I didn't catch those in the transition)
the order was changed to reader->monitor->resample (romangrothausmann@e41fff8#diff-50f53fc01f72ba938548d64a1218589bR121 and romangrothausmann@e41fff8#diff-50f53fc01f72ba938548d64a1218589bR175) which is great and (I understand now), same for itkResampleImageTest7.cxx (romangrothausmann@e41fff8#diff-dfa5983cf67253ce0e03aca56a207db2R91).
that ClearPipelineSavedInformation was introduced (e.g. romangrothausmann@e41fff8#diff-dfa5983cf67253ce0e03aca56a207db2R137)
VerifyAllInputCanStream was changed from 1 to 8: romangrothausmann@e41fff8#diff-50f53fc01f72ba938548d64a1218589bR181
Modified() is called on image instead of resample romangrothausmann@e41fff8#diff-dfa5983cf67253ce0e03aca56a207db2R137

I think all are fine except 5, where I intended that the monitor filter checks that streaming took not place, which I thought VerifyAllInputCanStream(1) would do. That and some minor corrections are proposed in #468.

Just for my better understanding: What is point 4. for, and why the change of 6.?

This reverts commit e5d421f. because InsightSoftwareConsortium#392 (replaced InsightSoftwareConsortium#82) was merged in bc4e197

This reverts commit e5d421f. because InsightSoftwareConsortium#392 (replaced InsightSoftwareConsortium#84) was merged in bc4e197

Squashed commit of the following: commit 2cdc0ee Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Feb 5 12:56:06 2019 +0100 Revert "ENH: new test are expected to fail as long as:": This reverts commit e5d421f. because InsightSoftwareConsortium#392 (replaced InsightSoftwareConsortium#84) was merged in bc4e197 commit d62cbd4 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Fri Sep 28 11:55:28 2018 +0200 BUG: use ITK_NULLPTR instead of identityTransform commit 9c0b04c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Sep 27 12:45:50 2018 +0200 ENH: add transform to ImageAlgorithm::EnlargeRegionOverBox commit 5b8eaf5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:07:50 2018 +0200 ENH: use ImageAlgorithm::EnlargeRegionOverBox instead commit d6c44ea Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Aug 30 13:05:31 2018 +0200 ENH: Method to compute InputRequestedRegion in itkResampleImageFilter based on suggestions from: https://discourse.itk.org/t/why-resampleimagefilter-is-slow/1217/14 https://itk.org/pipermail/insight-users/2015-April/051877.html and code from itkImageAlgorithm of v4.13.1 (based on commit 8510db2)

romangrothausmann · 2019-02-05T12:34:55Z

@thewtex Just noticed that for itkResampleImageTest2Streaming the comparison to the output of itkResampleImageTest2 was removed:
https://github.com/InsightSoftwareConsortium/ITK/compare/29f4e00f6883b07119df55bc9b7565f03d51df9c..69bb0bf11e345aaeeb4f72066d53b8d822061411#diff-e496b4f78280279f13a386cdbc983d41L255
Is that intended? My idea was that the itkResampleImageTest2Streaming return value and the comparison with the output from itkResampleImageTest2 should determine failure or success of the test.

thewtex · 2019-02-06T03:50:46Z

@romangrothausmann the comparison could be added back. If the test depends on the output of itkResampleImageTest2, we will need to add DEPENDS property to ensure itkResampleImageTest2 is executed first hen running in parallel.

@thewtex

Squashed commit of the following: commit ae8eb62 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Feb 7 15:56:57 2019 +0100 BUG: corrections from @thewtex in PR InsightSoftwareConsortium#469 commit 2cdc0ee Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Feb 5 12:56:06 2019 +0100 Revert "ENH: new test are expected to fail as long as:": This reverts commit e5d421f. because InsightSoftwareConsortium#392 (replaced InsightSoftwareConsortium#84) was merged in bc4e197 commit d62cbd4 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Fri Sep 28 11:55:28 2018 +0200 BUG: use ITK_NULLPTR instead of identityTransform commit 9c0b04c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Sep 27 12:45:50 2018 +0200 ENH: add transform to ImageAlgorithm::EnlargeRegionOverBox commit 5b8eaf5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:07:50 2018 +0200 ENH: use ImageAlgorithm::EnlargeRegionOverBox instead commit d6c44ea Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Aug 30 13:05:31 2018 +0200 ENH: Method to compute InputRequestedRegion in itkResampleImageFilter based on suggestions from: https://discourse.itk.org/t/why-resampleimagefilter-is-slow/1217/14 https://itk.org/pipermail/insight-users/2015-April/051877.html and code from itkImageAlgorithm of v4.13.1 (based on commit 8510db2)

…uccess: relates to InsightSoftwareConsortium#392

This reverts commit e5d421f. because InsightSoftwareConsortium#392 (replaced InsightSoftwareConsortium#84) was merged in bc4e197

romangrothausmann force-pushed the test_RSI-SDI_master branch from 9c5eced to 32ac3af Compare January 10, 2019 13:36

romangrothausmann mentioned this pull request Jan 10, 2019

WIP: Tests for "streaming for resample image filter in case of linear transforms" #84

Closed

dzenanz reviewed Jan 10, 2019

View reviewed changes

Modules/Filtering/ImageGrid/test/itkResampleImageTest7.cxx Outdated Show resolved Hide resolved

thewtex self-requested a review January 10, 2019 18:59

romangrothausmann force-pushed the test_RSI-SDI_master branch 2 times, most recently from 5532496 to 92e3d1e Compare January 23, 2019 12:55

romangrothausmann force-pushed the test_RSI-SDI_master branch from 92e3d1e to 2c91209 Compare January 23, 2019 14:13

romangrothausmann changed the title ~~ENH: new tests to verify streaming capabilities of itkResampleImageFilter~~ WIP: new tests to verify streaming capabilities of itkResampleImageFilter Jan 24, 2019

romangrothausmann force-pushed the test_RSI-SDI_master branch from 2c91209 to 8f4d75e Compare January 24, 2019 09:45

romangrothausmann force-pushed the test_RSI-SDI_master branch from 8f4d75e to 29f4e00 Compare January 24, 2019 10:50

jhlegarreta reviewed Feb 4, 2019

View reviewed changes

Modules/Filtering/ImageGrid/test/itkResampleImageTest7.cxx Show resolved Hide resolved

jhlegarreta mentioned this pull request Feb 4, 2019

ENH: Improve itk::ResampleImageFilter class coverage. #461

Merged

thewtex force-pushed the test_RSI-SDI_master branch from 29f4e00 to 69bb0bf Compare February 4, 2019 20:28

romangrothausmann and others added 3 commits February 4, 2019 16:31

STYLE: Remove unnecessary ResampleImageFilter input/output checks

df3129a

thewtex force-pushed the test_RSI-SDI_master branch from 69bb0bf to df3129a Compare February 4, 2019 21:32

thewtex changed the title ~~WIP: new tests to verify streaming capabilities of itkResampleImageFilter~~ new tests to verify streaming capabilities of itkResampleImageFilter Feb 5, 2019

thewtex approved these changes Feb 5, 2019

View reviewed changes

thewtex merged commit bc4e197 into InsightSoftwareConsortium:master Feb 5, 2019

romangrothausmann mentioned this pull request Feb 5, 2019

BUG: corrected logic for checking that streaming was not used #468

Merged

romangrothausmann mentioned this pull request Feb 5, 2019

WIP: streaming for resample image filter in case of linear transforms #82

Closed

romangrothausmann mentioned this pull request Feb 5, 2019

Streaming for itkResampleImageFilter in case of linear transforms #469

Merged

romangrothausmann deleted the test_RSI-SDI_master branch February 5, 2019 13:24

romangrothausmann added a commit to romangrothausmann/ITK that referenced this pull request Feb 7, 2019

ENH: compare outputs of itkResampleImageTest2Streaming to determine s…

dcd5535

…uccess: relates to InsightSoftwareConsortium#392

romangrothausmann mentioned this pull request Feb 7, 2019

ENH: compare outputs of itkResampleImageTest2Streaming to determine success #480

Merged

Uh oh!

Conversation

romangrothausmann commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romangrothausmann commented Jan 10, 2019

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romangrothausmann commented Jan 23, 2019

Uh oh!

romangrothausmann commented Jan 23, 2019

Uh oh!

romangrothausmann commented Jan 23, 2019

Uh oh!

romangrothausmann commented Jan 24, 2019

Uh oh!

romangrothausmann commented Jan 24, 2019

Uh oh!

thewtex commented Jan 24, 2019

Uh oh!

Uh oh!

thewtex commented Feb 4, 2019

Uh oh!

thewtex commented Feb 4, 2019

Uh oh!

romangrothausmann commented Feb 5, 2019

Uh oh!

romangrothausmann commented Feb 5, 2019

Uh oh!

thewtex commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

romangrothausmann commented Jan 10, 2019 •

edited

Loading