Skip to content

ENH: Support for VariableLengthVector in CastImageFilter#2094

Merged
thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
Leengit:variablelengthvector_constructor
Nov 11, 2020
Merged

ENH: Support for VariableLengthVector in CastImageFilter#2094
thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
Leengit:variablelengthvector_constructor

Conversation

@Leengit
Copy link
Contributor

@Leengit Leengit commented Nov 4, 2020

This pull request resolves the issue that the use of the OutputPixelType constructor in itkCastImageFilter::DynamicThreadedGenerateDataDispatched does not work when the pixel type is a VariableLengthVector.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but style requires slightly different whitespace. Please run clang-format on the changed source files.

@Leengit
Copy link
Contributor Author

Leengit commented Nov 4, 2020

@dzenanz, clang-format is trying to change, for example,

struct PixelFactory< itk::VariableLengthVector< TSubPixel > >

to

struct PixelFactory<itk::VariableLengthVector<TSubPixel>>

Is the latter the desired style? (Has the style guide been updated recently?)

@dzenanz
Copy link
Member

dzenanz commented Nov 4, 2020

The style is what clang-format 8.0.2 produces. We agreed not to fight it. Major style update was done maybe a year ago.

@blowekamp
Copy link
Member

Can you please add a test case for the conversion you are adding to the filter?

The filter works for variable length -> variable length. Based you the commit/PR message its not clear what conversion you are trying to add but I think it's something like fix array -> variable length.


inputIt.GoToBegin();
outputIt.GoToBegin();
OutputPixelType value{ Self::PixelFactory<OutputPixelType>::New(componentsPerPixel) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pixel factory is not needed. Just do the following:

OutputPixelType value = outputIt.Get();

I believe the above should be a "deep copy" but please test the output in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@Leengit
Copy link
Contributor Author

Leengit commented Nov 4, 2020

Can you please add a test case for the conversion you are adding to the filter?

Will do.

The filter works for variable length -> variable length. Based you the commit/PR message its not clear what conversion you are trying to add but I think it's something like fix array -> variable length.

A comment by @blowekamp in Issue #1579 suggests that casting to a VectorImage did not work in CastImageFilter<TInputImage, TOutputImage>::DynamicThreadedGenerateDataDispatche due to an invalid C++ construction of an OutputPixelType. This PR fix addresses this.

Yes, the Image<Vector<type,c>,d>VectorImage<type,d> was just implemented, in PR #2073.

Edit: oops just realized that the person I am responding to and the supplier of the original comment are one and the same, @blowekamp. Apologies if the above reads kind of crazy due to my overlooking that!

@blowekamp
Copy link
Member

The simplicity of this solution was missed when I looked at this. Thank you for changing to the simpler solution and adding tests.

With this conversion: VectorImage<type,d> ↔ Image<Vector<type,c>,d> it may be worth wild to test/verify, that an exception will be thrown ( early in the pipeline e.g. GenerateOutputInformation? ) when the dimensions of the vector-like objects don't match.

@Leengit
Copy link
Contributor Author

Leengit commented Nov 5, 2020

With this conversion: VectorImage<type,d> ↔ Image<Vector<type,c>,d> it may be worthwhile to test/verify, that an exception will be thrown ( early in the pipeline e.g. GenerateOutputInformation? ) when the dimensions of the vector-like objects don't match.

I believe that the desired behavior is to do something reasonable even when the dimensions of the input and and output images are not equal. For example, the comment at the top of of the definition of CastImageFilter::GenerateOutputInformation() is:

// do not call the superclass' implementation of this method since
// this filter allows the input the output to be of different dimensions

@blowekamp
Copy link
Member

With this conversion: VectorImage<type,d> ↔ Image<Vector<type,c>,d> it may be worthwhile to test/verify, that an exception will be thrown ( early in the pipeline e.g. GenerateOutputInformation? ) when the dimensions of the vector-like objects don't match.

I believe that the desired behavior is to do something reasonable even when the dimensions of the input and and output images are not equal. For example, the comment at the top of of the definition of CastImageFilter::GenerateOutputInformation() is:

// do not call the superclass' implementation of this method since
// this filter allows the input the output to be of different dimensions

There are two "dimensions":

  • The dimension of the Image ( which your comment refers too )
  • The dimension/size/length of the pixel component vector ( which my comment refers too ).

If the length of the input pixel vector and the output vector differ we need to ensure we have "well defined" behavior and don't over run any buffers. My expectation would be an exception would be thrown some where under the GenerateOutputInformation when it is detected that length of the pixel vectors are not equal.

@Leengit
Copy link
Contributor Author

Leengit commented Nov 5, 2020

If the length of the input pixel vector and the output vector differ we need to ensure we have "well defined" behavior and don't over run any buffers. My expectation would be an exception would be thrown some where under the GenerateOutputInformation when it is detected that length of the pixel vectors are not equal.

Ah, yes, of course, thank you. Will do.

@Leengit Leengit marked this pull request as draft November 6, 2020 18:21
@Leengit Leengit marked this pull request as ready for review November 10, 2020 16:26
@thewtex thewtex merged commit c0ae3ef into InsightSoftwareConsortium:master Nov 11, 2020
@Leengit Leengit deleted the variablelengthvector_constructor branch March 5, 2021 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants