KitwareMedical / ITKUltrasound

ITK module with classes particularly useful for ultrasound.
http://www.insight-journal.org/browse/publication/722
Apache License 2.0
50 stars 22 forks source link

ENH: Use ITK reusable workflow #214

Closed tbirdso closed 1 year ago

tbirdso commented 1 year ago

Consolidates CI build/test/package actions in common, reusable ITK external module workflow.

tbirdso commented 1 year ago

cxx-build-workflow appears to catch a valid warning where a pointer with null value is used in itkSliceSeriesSpecialCoordinatesImage.h.

https://open.cdash.org/viewBuildError.php?type=1&buildid=8372431

https://github.com/KitwareMedical/ITKUltrasound/blob/master/include/itkSliceSeriesSpecialCoordinatesImage.h#L477

I'm unclear on why these warnings did not appear in previous CI runs. It would be good to address the issue, though at a glance I am not sure what the proper fix would be.

@dzenanz are you perhaps familiar with this class and have an idea of what the block in question is trying to do in line 477 and subsequent null transform uses?

dzenanz commented 1 year ago

Commit b40c67df9cc8daa7f77e0d4dc6654dcd77077fc9 introduced the problem.

dzenanz commented 1 year ago

Perhaps add transform = this->GetSliceTransform(ceil); inside the else block starting at line 472: https://github.com/KitwareMedical/ITKUltrasound/blob/ea0ff89c7e8cb36f766abc37d452596d2ecf7944/include/itkSliceSeriesSpecialCoordinatesImage.h#L472

tbirdso commented 1 year ago

After taking a closer look I am concerned about the procedure for attempting to extrapolate outside of the image extent represented with slice transforms.

itk::SliceSeriesSpecialCoordinatesImage::TransformIndexToPhysicalPoint appears to attempt to handle indices that lie outside the image extent along the N-1th pixel dimension by setting that element in point to the pixel distance between the requested index and the known image extent, then applying the slice transform. However, point is intended to already represent a physical point in (N-1)D space, so this is mixing indices with physical measurements.

https://github.com/KitwareMedical/ITKUltrasound/blob/master/include/itkSliceSeriesSpecialCoordinatesImage.h#L569

I believe the behavior in TransformIndexToPhysicalPoint was intended to be applied to TransformContinuousIndexToPhysicalPoint where the null-transform warning occurs. However, it seems like there is more fundamental discussion required to verify and/or update itk::SliceSeriesSpecialCoordinatesImage.

I propose the current PR be merged which correctly identifies warnings in ITKUltrasound C++ classes, and a subsequent issue/PR be opened to track discussion around fixes for itk::SliceSeriesSpecialCoordinatesImage.

dzenanz commented 1 year ago

The current CI is green. We should merge this if we decided we will tackle this issue immediately afterwards. Otherwise it will hamper other PRs. If therefore prefer a fix for the warning to be merged before this PR.

tbirdso commented 1 year ago

Otherwise it will hamper other PRs. If therefore prefer a fix for the warning to be merged before this PR.

That's fair. I will add a warning exception for now and log the warning as an issue to be followed up on as time allows. To my knowledge itk::SliceSeriesSpecialCoordinatesImage is not heavily used and is not wrapped for Python.

dzenanz commented 1 year ago

That sounds good.