Skip to content

ENH: Enable writing scl_slope and scl_inter in NIfTI header#47

Closed
Hakkk2002 wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
Hakkk2002:nifti_scale
Closed

ENH: Enable writing scl_slope and scl_inter in NIfTI header#47
Hakkk2002 wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
Hakkk2002:nifti_scale

Conversation

@Hakkk2002
Copy link
Contributor

@Hakkk2002 Hakkk2002 commented May 17, 2018

This is useful when writing image data that are stored in integers.

Change-Id: I72caaf4565a195b2d603b95a01eb1ff5ccfddbda

@itkrobot
Copy link
Collaborator

itkrobot commented May 17, 2018

Can one of the admins verify this patch?

2 similar comments
@itkrobot
Copy link
Collaborator

itkrobot commented May 17, 2018

Can one of the admins verify this patch?

@itkrobot
Copy link
Collaborator

itkrobot commented May 17, 2018

Can one of the admins verify this patch?

thewtex
thewtex requested changes May 26, 2018
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @Hakkk2002 !

Could the new functionality please be covered in the regression tests?

@Hakkk2002
Copy link
Contributor Author

Hakkk2002 commented May 29, 2018

The check seemed to fail somewhere else in the then master branch this one based.

I add these functions for the following scenario:
Suppose an Image<float, 3u> is going to be written in NIfTI format but in int16 voxel datatype for example. Then the image could first pass through RescaleIntensityImageFilter<Image<float, 3u>, Image<int16, 3u>> with proper output min/max settings, and then to the NIfTI writer. In addition, the scl_scale and scl_intercept can be derived from GetScale() and GetShift() of the rescale filter, passed to the NiftiImageIO via these two Set functions, and written to the NIfTI image header.

@thewtex
Copy link
Member

thewtex commented May 29, 2018

@Hakkk2002 yes, the current CI test failures are unrelated. Please rebase your branch on master -- it should help with the false positives.

Yes, processing that modifies the intensity distribution should be thought about carefully.

CC: @hjmjohnson

@Hakkk2002 Hakkk2002 force-pushed the nifti_scale branch 2 times, most recently from dd8e1a3 to 4abe9b5 Compare May 31, 2018 06:47
@thewtex
Copy link
Member

thewtex commented Jun 6, 2018

Thanks for the rebase @Hakkk2002 , tests are passing now :-)

Could you please add a regression test for this new functionality? This will ensure it is working as expected and that it continuous to work as expected.

ITK/Modules/IO/NIFTI/test/itkExtractSlice.cxx could be extended to take a slope and intercept command line argument:

https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/IO/NIFTI/test/itkExtractSlice.cxx

Then another test could be added define in the test/CMakeLists.txt that checks the use of a RescaleIntercept and a RescaleSlope that differ from the default:

ITK/Modules/IO/NIFTI/test/CMakeLists.txt

Lines 77 to 79 in ee4488b

itk_add_test(NAME itkExtractSliceSlopeInterceptUCHAR
COMMAND ITKIONIFTITestDriver --compare DATA{Baseline/SlopeInterceptUCHAR-midSlice.nrrd} ${ITK_TEST_OUTPUT_DIR}/SlopeInterceptUCHAR-midSlice.nrrd
itkExtractSlice DATA{Input/SlopeInterceptUCHAR.nii.gz} ${ITK_TEST_OUTPUT_DIR}/SlopeInterceptUCHAR-midSlice.nrrd)

@Hakkk2002
Copy link
Contributor Author

Hakkk2002 commented Jun 7, 2018

I will work on a test later, but be aware that only the NIfTI writing with slope/intercept is new; the rescaling and shifting have already been implemented in the NIfTI reading process. So just to extract (read) a slice from a NIfTI file may not be relevant.

@thewtex
Copy link
Member

thewtex commented Jun 7, 2018

@Hakkk2002 great. Yes, we will also have to compare the written out file. This is achieved with the --compare argument to the test driver (see the referenced ITK/Modules/IO/NIFTI/test/CMakeLists.txt file).

@Hakkk2002
Copy link
Contributor Author

Hakkk2002 commented Jun 29, 2018 •
edited
Loading

A test has been appended to Test11 of itkNiftiImageIO in ITK/Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx.
The existing Test11 writes NIfTI files with a slope and intercept by using the native nifti1_io, reads them with itkNiftiImageIO, and compares the pixel values.
The appended new part does the same in general but writes NIfTI files by itkNiftiImageIO with the new SetRescaleSlope and SetRescaleIntercept functions instead of by nifti1_io.

@Hakkk2002 Hakkk2002 force-pushed the nifti_scale branch 4 times, most recently from 6ed38b4 to e610663 Compare July 4, 2018 07:06
thewtex
thewtex requested changes Jul 4, 2018
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Well done adding the regression tests.

A few missing typename keywords are noted inline.

Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx Outdated
// fill out an image and write it as nifti.
const char *filename = "SlopeIntercept.nii";
typedef typename itk::Image<PixelType,3> OutputImageType;
OutputImageType::RegionType region;
Copy link
Member

@thewtex thewtex Jul 4, 2018

Choose a reason for hiding this comment

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

Since OuputImageType::RegionType is a dependent type this needs the typename keyword.

Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx Outdated
const char *filename = "SlopeIntercept.nii";
typedef typename itk::Image<PixelType,3> OutputImageType;
OutputImageType::RegionType region;
OutputImageType::IndexType start;
Copy link
Member

@thewtex thewtex Jul 4, 2018

Choose a reason for hiding this comment

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

typename keyword

Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx Outdated
start[0] = 0;
start[1] = 0;
start[2] = 0;
OutputImageType::SizeType size;
Copy link
Member

@thewtex thewtex Jul 4, 2018

Choose a reason for hiding this comment

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

typename keyword

Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx Outdated
size[2] = 4;
region.SetSize(size);
region.SetIndex(start);
OutputImageType::Pointer outputimage = OutputImageType::New();
Copy link
Member

@thewtex thewtex Jul 4, 2018

Choose a reason for hiding this comment

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

typename keyword

Modules/IO/NIFTI/test/itkNiftiImageIOTest5.cxx Outdated
itout.Set(static_cast<PixelType>(i));
}
typedef itk::ImageFileWriter< OutputImageType > WriterType;
WriterType::Pointer writer = WriterType::New();
Copy link
Member

@thewtex thewtex Jul 4, 2018

Choose a reason for hiding this comment

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

typename keyword

@Hakkk2002
ENH: Enable writing scl_slope and scl_inter in NIfTI header
1000e85 
This is useful when the image data are stored in integers.

Change-Id: I5d7ca0f93fcabccc6ee2136cf2da3a2a3f08d3bd
thewtex
thewtex approved these changes Jul 16, 2018
View reviewed changes
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Pushed to Gerrit:

http://review.source.kitware.com/#/c/23566/

@thewtex
Copy link
Member

thewtex commented Jul 16, 2018

Merged to master -- thanks for contributing!

@thewtex thewtex closed this Jul 16, 2018
@Hakkk2002 Hakkk2002 deleted the nifti_scale branch July 16, 2018 07:06
@hjmjohnson hjmjohnson mentioned this pull request Apr 11, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thewtex thewtex thewtex approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Hakkk2002 @itkrobot @thewtex