Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Add template for input ImageType for itk2Dicom #507

Merged
merged 2 commits into from
Aug 28, 2024

Conversation

jadh4v
Copy link
Collaborator

@jadh4v jadh4v commented Aug 16, 2024

No description provided.

@fedorov
Copy link
Member

fedorov commented Aug 21, 2024

@jadh4v: @michaelonken looked at this and noticed something that I did not think about.

I think it's fine as long as VectorImageAdapter (i.e. itk::VectorImageToImageAdaptor<short, 3U>) is a compatible to ShortImageType which I don't know. If VectorImageAdapter includes pixel data types that we cannot handle it should be checked somewhere in the code before starting conversions.

Can you please comment on this?

@jadh4v
Copy link
Collaborator Author

jadh4v commented Aug 21, 2024

@jadh4v: @michaelonken looked at this and noticed something that I did not think about.

I think it's fine as long as VectorImageAdapter (i.e. itk::VectorImageToImageAdaptor<short, 3U>) is a compatible to ShortImageType which I don't know. If VectorImageAdapter includes pixel data types that we cannot handle it should be checked somewhere in the code before starting conversions.

Can you please comment on this?

Good catch. The use cases are limited by explicit instantiation of the templated function itkimage2dcmSegmentation. So we only expect two versions of the function to be used through the compiled library. However nothing stops someone from instantiating the function for other pixelTypes when building from source.
We have two options:

  1. We could check for PixelType to be short during runtime and throw error that we don't handle the conversion.
  2. Write the templated function so that it only allows short pixeltype during compile time. Any attempt to instantiate the function with a non-short type will result in a compile time error.

@jadh4v
Copy link
Collaborator Author

jadh4v commented Aug 21, 2024

I have pushed a commit adding the restriction to the template function.
Trying to instantiate the function with say VectorImageToImageAdaptor<int, 3U> with int as PixelType will result in the following error:

error C3190: 'DcmDataset *dcmqi::Itk2DicomConverter::itkimage2dcmSegmentation(std::vector<DcmDataset *,std::allocator<DcmDataset *>>,std::vector<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>,std::allocator<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>>>,const std::string &,bool,bool,bool)'
with the provided template arguments is not the explicit instantiation of any member function of 'dcmqi::Itk2DicomConverter' 
dcmqi\libsrc\Itk2DicomConverter.cpp(713,44): error C2945: explicit instantiation does not refer to a template-class specialization

@jadh4v
Copy link
Collaborator Author

jadh4v commented Aug 21, 2024

Hmmm will have to add fixes for Linux and macOS.

@michaelonken
Copy link
Member

I have pushed a commit adding the restriction to the template function. Trying to instantiate the function with say VectorImageToImageAdaptor<int, 3U> with int as PixelType will result in the following error:

error C3190: 'DcmDataset *dcmqi::Itk2DicomConverter::itkimage2dcmSegmentation(std::vector<DcmDataset *,std::allocator<DcmDataset *>>,std::vector<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>,std::allocator<slicer_itk::SmartPointer<const slicer_itk::VectorImageToImageAdaptor<int,3>>>>,const std::string &,bool,bool,bool)'
with the provided template arguments is not the explicit instantiation of any member function of 'dcmqi::Itk2DicomConverter' 
dcmqi\libsrc\Itk2DicomConverter.cpp(713,44): error C2945: explicit instantiation does not refer to a template-class specialization

👍 I think that's the best solution to already reject wrong usage during compile time.

…e2dcmSegmentation

Tempalted function Itk2DicomConverter::itkimage2dcmSegmentation should
not be instantiated with PixelType other than short. This is because
we do not handle any other pixel type for writing DICOM segmentation
object.

Any attempt to instantiate should result in a compile time error,
warning the developer about the limitation.
@jadh4v
Copy link
Collaborator Author

jadh4v commented Aug 28, 2024

@michaelonken @fedorov Looks like the CI is passing with my last fix. Please approve and merge.

FYI: @jcfr @thewtex

@fedorov fedorov merged commit bf6d622 into QIICR:master Aug 28, 2024
7 checks passed
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.

3 participants