WIP: Tests for "streaming for resample image filter in case of linear transforms" by romangrothausmann · Pull Request #84 · InsightSoftwareConsortium/ITK

romangrothausmann · 2018-10-25T10:32:30Z

PR for only the tests that are expected to succeed with #82 in order to check their outcome without the changes to itkResampleImageFilter (which currently appear to effect other tests (like itkMultiResolutionPyramidImageFilterWithResampleFilterTest) as well)

…geTest2

… imaging (SDI) and without SDI

thewtex · 2019-01-02T16:16:19Z

@romangrothausmann Happy New Year! 🍾

One of my resolutions is to get this awesome contribution in :-)

Would you mind, please

Squash all commits into a single ENH commit.
Rebase on current ITK master
Create the pull request against ITK master instead of release-4.13

We have the CI working on master, so that should help us move forward.

Thanks!

jhlegarreta

Nice job. Some specific comments in-line to improve the tests, save typing, etc.

romangrothausmann · 2019-01-10T14:43:54Z

Created PR #392 according to #84 (comment).
@thewtex Just for better understanding how contribs are wanted:
Why ENH and not TEST in this case?
Why squash all into one? I always find it a pity to loose the context to the changes. With just one commit, I already have difficulties to see how those changes built up and the reason behind.

@jhlegarreta Thanks for all your valuable comments. The code of itkResampleImageTest7.cxx is based on that from itkResampleImageTest2.cxx and itkWarpImageFilterTest2.cxx. The lines you are referring to are mostly from itkResampleImageTest2.cxx, which seem to be the same on master. What should I conform to?

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 for the PR that is based on master?

jhlegarreta · 2019-01-10T14:56:11Z

@jhlegarreta Thanks for all your valuable comments. The code of itkResampleImageTest7.cxx is based on that from itkResampleImageTest2.cxx and itkWarpImageFilterTest2.cxx. The lines you are referring to are mostly from itkResampleImageTest2.cxx, which seem to be the same on master. What should I conform to?

@romangrothausmann It is true that many tests in master still do not benefit from the use of the macros, but for any new test we write or any test we modify, I'd say that we should converge to using them for the reasosn stated.

thewtex · 2019-01-10T19:08:34Z

Created PR #392 according to #84 (comment).

@romangrothausmann fantastic, thanks! Should we close this PR, then?

Why ENH and not TEST in this case?

We have a pre-defined set of commit subject prefixes, such as ENH, so we are using a common ontology. These are described here:

https://github.com/InsightSoftwareConsortium/ITK/blob/master/CONTRIBUTING.md#create-a-topic

Why squash all into one?

Once it comes time for someone else to review, it is very helpful resolve the story versus presenting its evolution. This prevents reviewer from identifying issues in a commit that may be fixed or changed later.

Also, the tree is always in a good, working state, no matter what commit is checked out. And, porting bug fixes and features to another branch is possible with git cherry-pick.

romangrothausmann · 2019-01-23T10:11:25Z

Thanks @jhlegarreta and @thewtex for your replies.

We have a pre-defined set of commit subject prefixes, such as ENH, so we are using a common ontology. These are described here:

I'm sorry, I thought there would be a subject prefix TEST, must have mixed that up from some other project. Since TEST isn't give, ENH is definitely the best fitting.

Once it comes time for someone else to review, it is very helpful resolve the story versus presenting its evolution. This prevents reviewer from identifying issues in a commit that may be fixed or changed later.

I see and am happy to adapt.
The only thing I missing though when going through the commits of others is that with this squashing the relation to some other code that was copied to be modified later is not visible any more, like for example the transition visible between these two:
romangrothausmann@0a0d9a1
romangrothausmann@f768f4d
Is there some other git way (I'm not aware of) to visualize the changes as shown by
romangrothausmann@f768f4d
instead of just a new file consisting only of newly added lines? I.e. a trace that shows that most of the code is a duplicate from a working base? Wouldn't that help a lot in ruling out what lines/changes could have introduced a problem?
With the two commits given above, we can easily rule out any problems from romangrothausmann@0a0d9a1 as it is a plane copy (told so by git or gitk but sadly not GH) so any problems in romangrothausmann@f768f4d could only originate from the very few changes introduced within that commit. In contrast to
romangrothausmann@32ac3af#diff-8aa2bf916025440064f4ee778c16cebc
where no changes are visible because all lines are new.

see InsightSoftwareConsortium#84 (review)

jhlegarreta · 2019-01-23T14:04:17Z

Thanks @romangrothausmann ! An inline comment on the boolean methods checks.

see InsightSoftwareConsortium#84 (review)

@jhlegarreta

…lter Squashed commit of the following: commit 6b59a58 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Jan 24 10:28:29 2019 +0100 ENH: new test are expected to fail as long as: itkResampleFilter does not support streaming for linear transforms GH PR InsightSoftwareConsortium#82 commit 9b63a48 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 14:45:46 2019 +0100 ENH: improvements suggested by @jhlegarreta: see InsightSoftwareConsortium#84 (review) commit 1b922ae Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Oct 23 08:26:39 2018 +0200 ENH: mark resample as modified to enforce re-execution of filter chain commit a6682b0 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 13:17:00 2019 +0100 ENH: test for comparing output with stream driven imaging (SDI) and without based on itkWarpImageFilterTest2.cxx and itkResampleImageTest.cxx commit ff8e5df Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 15:24:04 2018 +0200 ENH: plain copy of itkResampleImageTest4.cxx and its cmake commit 97f3073 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:16:55 2018 +0200 ENH: verify streaming for linear case commit 2e871f1 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:05:58 2018 +0200 TEST: demand streaming and employ where possible commit ca68f7f Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Sep 26 15:05:43 2018 +0200 TEST: write MHAs (which support streaming) instead of PNGs commit 56b6c25 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:57:59 2018 +0200 TEST: employ itkResampleImageTest2s, still the same as itkResampleImageTest2 commit 4a20d6a Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:54:50 2018 +0200 ENH: plain copy of itkResampleImageTest2.cxx and its cmake

@jhlegarreta

…lter Squashed commit of the following: commit e5d421f Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Jan 24 10:28:29 2019 +0100 ENH: new test are expected to fail as long as: itkResampleFilter does not support streaming for linear transforms GH PR InsightSoftwareConsortium#82 commit 6067a02 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 14:45:46 2019 +0100 ENH: improvements suggested by @jhlegarreta: see InsightSoftwareConsortium#84 (review) commit 4118d20 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Oct 23 08:26:39 2018 +0200 ENH: mark resample as modified to enforce re-execution of filter chain commit 375eac5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 13:17:00 2019 +0100 ENH: test for comparing output with stream driven imaging (SDI) and without based on itkWarpImageFilterTest2.cxx and itkResampleImageTest.cxx commit a7f066a Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 15:24:04 2018 +0200 ENH: plain copy of itkResampleImageTest4.cxx and its cmake commit a7fbd1c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 13:51:59 2018 +0200 ENH: verify streaming is not possible for non-linear case commit 21771f8 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:16:55 2018 +0200 ENH: verify streaming for linear case commit 1c66653 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:05:58 2018 +0200 TEST: demand streaming and employ where possible commit 89fd06c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Sep 26 15:05:43 2018 +0200 TEST: write MHAs (which support streaming) instead of PNGs commit 18efabd Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:57:59 2018 +0200 TEST: employ itkResampleImageTest2s, still the same as itkResampleImageTest2 commit c4d64b5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:54:50 2018 +0200 ENH: plain copy of itkResampleImageTest2.cxx and its cmake

see InsightSoftwareConsortium#84 (review)

@jhlegarreta

…lter Squashed commit of the following: commit e5d421f Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Jan 24 10:28:29 2019 +0100 ENH: new test are expected to fail as long as: itkResampleFilter does not support streaming for linear transforms GH PR InsightSoftwareConsortium#82 commit 6067a02 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 14:45:46 2019 +0100 ENH: improvements suggested by @jhlegarreta: see InsightSoftwareConsortium#84 (review) commit 4118d20 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Oct 23 08:26:39 2018 +0200 ENH: mark resample as modified to enforce re-execution of filter chain commit 375eac5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 13:17:00 2019 +0100 ENH: test for comparing output with stream driven imaging (SDI) and without based on itkWarpImageFilterTest2.cxx and itkResampleImageTest.cxx commit a7f066a Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 15:24:04 2018 +0200 ENH: plain copy of itkResampleImageTest4.cxx and its cmake commit a7fbd1c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 13:51:59 2018 +0200 ENH: verify streaming is not possible for non-linear case commit 21771f8 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:16:55 2018 +0200 ENH: verify streaming for linear case commit 1c66653 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:05:58 2018 +0200 TEST: demand streaming and employ where possible commit 89fd06c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Sep 26 15:05:43 2018 +0200 TEST: write MHAs (which support streaming) instead of PNGs commit 18efabd Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:57:59 2018 +0200 TEST: employ itkResampleImageTest2s, still the same as itkResampleImageTest2 commit c4d64b5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:54:50 2018 +0200 ENH: plain copy of itkResampleImageTest2.cxx and its cmake

@jhlegarreta

…lter Squashed commit of the following: commit e5d421f Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Thu Jan 24 10:28:29 2019 +0100 ENH: new test are expected to fail as long as: itkResampleFilter does not support streaming for linear transforms GH PR InsightSoftwareConsortium#82 commit 6067a02 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 14:45:46 2019 +0100 ENH: improvements suggested by @jhlegarreta: see InsightSoftwareConsortium#84 (review) commit 4118d20 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Oct 23 08:26:39 2018 +0200 ENH: mark resample as modified to enforce re-execution of filter chain commit 375eac5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Jan 23 13:17:00 2019 +0100 ENH: test for comparing output with stream driven imaging (SDI) and without based on itkWarpImageFilterTest2.cxx and itkResampleImageTest.cxx commit a7f066a Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 15:24:04 2018 +0200 ENH: plain copy of itkResampleImageTest4.cxx and its cmake commit a7fbd1c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Mon Oct 22 13:51:59 2018 +0200 ENH: verify streaming is not possible for non-linear case commit 21771f8 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:16:55 2018 +0200 ENH: verify streaming for linear case commit 1c66653 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 15:05:58 2018 +0200 TEST: demand streaming and employ where possible commit 89fd06c Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Wed Sep 26 15:05:43 2018 +0200 TEST: write MHAs (which support streaming) instead of PNGs commit 18efabd Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:57:59 2018 +0200 TEST: employ itkResampleImageTest2s, still the same as itkResampleImageTest2 commit c4d64b5 Author: Roman Grothausmann <romangrothausmann@users.noreply.github.com> Date: Tue Sep 25 14:54:50 2018 +0200 ENH: plain copy of itkResampleImageTest2.cxx and its cmake

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)

@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)

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

romangrothausmann added 7 commits October 23, 2018 08:49

TEST: plain copy of itkResampleImageTest.cxx and its cmake

0a0d9a1

TEST: employ itkResampleImageTest2s, still the same as itkResampleIma…

f768f4d

…geTest2

TEST: write MHAs (which support streaming) instead of PNGs

3204117

TEST: demand streaming and employ where possible

a44c967

TEST: verify streaming for linear case

5d765b4

TEST: itkResampleImageTest7.cxx for comparing output of stream driven…

9cd71e9

… imaging (SDI) and without SDI

mark resample as modified to enforce re-execution of filter chain

0f962c5

jhlegarreta mentioned this pull request Oct 27, 2018

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

Closed

kwrobot added temp and removed temp labels Nov 6, 2018

thewtex requested a review from jhlegarreta January 2, 2019 16:16

jhlegarreta requested changes Jan 2, 2019

View reviewed changes

romangrothausmann mentioned this pull request Jan 10, 2019

new tests to verify streaming capabilities of itkResampleImageFilter #392

Merged

romangrothausmann closed this Jan 23, 2019

romangrothausmann added a commit to romangrothausmann/ITK that referenced this pull request Jan 23, 2019

ENH: improvements suggested by @jhlegarreta:

0c738df

see InsightSoftwareConsortium#84 (review)

romangrothausmann added a commit to romangrothausmann/ITK that referenced this pull request Jan 23, 2019

ENH: improvements suggested by @jhlegarreta:

9b63a48

see InsightSoftwareConsortium#84 (review)

romangrothausmann added a commit to romangrothausmann/ITK that referenced this pull request Jan 24, 2019

ENH: improvements suggested by @jhlegarreta:

6067a02

see InsightSoftwareConsortium#84 (review)

romangrothausmann deleted the test_RSI-SDI branch May 17, 2019 10:32

hjmjohnson mentioned this pull request Apr 11, 2020

BUG: Fix mattes metric with sse instructions #1762

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

WIP: Tests for "streaming for resample image filter in case of linear transforms"#84
romangrothausmann wants to merge 7 commits intoInsightSoftwareConsortium:release-4.13from
romangrothausmann:test_RSI-SDI

romangrothausmann commented Oct 25, 2018

Uh oh!

thewtex commented Jan 2, 2019 •

edited

Loading

Uh oh!

jhlegarreta left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romangrothausmann commented Jan 10, 2019

Uh oh!

jhlegarreta commented Jan 10, 2019

Uh oh!

thewtex commented Jan 10, 2019

Uh oh!

romangrothausmann commented Jan 23, 2019

Uh oh!

jhlegarreta commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

romangrothausmann commented Oct 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romangrothausmann commented Jan 10, 2019

Uh oh!

jhlegarreta commented Jan 10, 2019

Uh oh!

thewtex commented Jan 10, 2019

Uh oh!

romangrothausmann commented Jan 23, 2019

Uh oh!

jhlegarreta commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thewtex commented Jan 2, 2019 •

edited

Loading