Repository navigation
BUG: Fix DicomSeriesReader test for ITK PR#5357 direction fix - #2591
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the IO.DicomSeriesReader unit test to accommodate the ITK behavior change from InsightSoftwareConsortium/ITK#5357, where ForceOrthogonalDirection now negates the z-column of the direction matrix when reading a DICOM series in reverse order.
Changes:
- Replaces full-matrix
EXPECT_EQdirection comparison with element-wise checks. - Validates x/y direction columns are unchanged and z-column matches in magnitude (allowing sign differences across ITK versions).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| const auto d = image2.GetDirection(); | ||
| const auto dr = image2_reverse.GetDirection(); | ||
| ASSERT_EQ(d.size(), 9u); |
There was a problem hiding this comment.
Fixed: added ASSERT_EQ(d.size(), dr.size()) before ASSERT_EQ(d.size(), 9u) so a dimension mismatch between the two images is caught as a distinct pre-condition failure.
| EXPECT_NEAR(std::abs(d[i]), std::abs(dr[i]), 1e-10) | ||
| << "Direction z-column element [" << i << "] magnitude should be unchanged by ReverseOrder"; |
There was a problem hiding this comment.
Fixed: added #include <cmath> explicitly to make the std::abs dependency clear and avoid reliance on transitive includes.
| EXPECT_EQ(image2.GetDirection(), image2_reverse.GetDirection()); | ||
| // ITK PR#5357 fixed ForceOrthogonalDirection to negate the z-column when | ||
| // reading in reverse order. X/y-columns are unaffected; z-column must match | ||
| // in magnitude (sign may differ between old and new ITK). |
There was a problem hiding this comment.
Comment should refer to the ITK versions explicitly (<6.0.0 and >6.0.0), old and new will change over time.
There was a problem hiding this comment.
Fixed: updated the comment to reference explicit ITK versions — ITK < 6.0 (pre-fix, sign preserved) and ITK >= 6.0 (post ITK PR#5357, sign negated).
52563b5 to
6900990
Compare
Summary
The
IO.DicomSeriesReadertest was failing due to a behavioral change in ITK PR InsightSoftwareConsortium/ITK#5357, which fixedForceOrthogonalDirectionto correctly negate the z-column of the direction matrix when reading a DICOM series in reverse order.The previous test used
EXPECT_EQon the full direction matrix, expecting it to be identical between forward and reversed reads. This was correct for old ITK (where the z-column sign was not updated — a bug), but fails with the fixed ITK.Fix
Replace the single
EXPECT_EQ(direction, direction_reverse)with element-wise checks that are valid for both old and new ITK:ReverseOrder— these come from DICOM image orientation tags and are unaffected.