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

Respect pixel padding value during interpolation #263

Merged
merged 2 commits into from
Feb 1, 2019

Conversation

alund
Copy link
Contributor

@alund alund commented Jan 31, 2019

@neurolabusc
Copy link
Collaborator

This looks clear. Can I make one request: instead of four variables
isHasPixelPaddingValue
isHasFloatPixelPaddingValue
pixelPaddingValue
floatPixelPaddingValue
Could we have just
floatPixelPaddingValue
that is set to NAN by default? The structure TDICOMdata is required for each and every DICOM, so keeping these smaller is better. For the case of 16-bit data you can just use the specify pixelPaddingValue = round(floatPixelPaddingValue) in nii_saveNII3Dtilt(). And isHasPixelPaddingValue can be set in nii_saveNII3Dtilt() and nii_saveNII3DtiltFloat32() based on whether floatPixelPaddingValue == NAN.

* Fixes rordenlab#262
* Use a single float pixelPaddingValue in TDICOMData rather than
  pixelPaddingValue, floatPixelPaddingValue, isHasPixelPaddingValue and
  isHasFloatPixelPaddingValue
* Read kFloatPixelPaddingValue with dcmFloat() rather than dcmStrFloat()
* Use pixel padding value when available for padding added during tilt
  correction.
@alund
Copy link
Contributor Author

alund commented Feb 1, 2019

I have pushed a new commit that makes this change. It also fixes a mistake I made: the data type for Float Pixel Padding Value is FL and so should have been read with dcmFloat() not dcmStrFloat(). I also realized that, if a pixel padding value is defined, it should be used for the padding done in nii_saveNII3Dtilt() and nii_saveNII3DtiltFloat32().

Having done all of that, I would like to do some more testing before you merge this. Feel free to review, but even if you have no more comments, I would appreciate it if you wait before merging until I have confirmed that my additional testing is complete.

@neurolabusc
Copy link
Collaborator

Looks good to me. I will hold off until I get your OK.

@alund
Copy link
Contributor Author

alund commented Feb 1, 2019

The tests I have run so far have been fine. I think you can go ahead and merge. Obviously if I find anything later, I'll fix it and submit another pull request.

@neurolabusc neurolabusc merged commit a28cc7f into rordenlab:development Feb 1, 2019
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.

2 participants