Skip to content

BUG: Deprecate DWIConvert; fix #294, #370, #457; document #342, #412#572

Merged
hjmjohnson merged 5 commits into
mainfrom
fix/dwi-convert-sunset
Apr 5, 2026
Merged

BUG: Deprecate DWIConvert; fix #294, #370, #457; document #342, #412#572
hjmjohnson merged 5 commits into
mainfrom
fix/dwi-convert-sunset

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Apr 5, 2026

Copy link
Copy Markdown
Member

Summary

  • Add a deprecation notice to DWIConvert/README.md directing users to dcm2niix
  • Document that DWIConvert will be moved to ARCHIVE/ in a future release
  • List known open issues that will not be fixed, with migration guidance

Issues addressed

This PR serves as the sunset notice for DWIConvert. The following issues
should be closed once this is merged, as they will not be fixed:

Context

As noted by the maintainer on #457, DWIConvert has been superseded by
dcm2niix which has broader community support and better maintenance.
Rather than fixing these long-standing issues in a tool that is being
retired, this PR documents the deprecation and points users to the
recommended replacement.

Test plan

  • Verify README.md renders correctly on GitHub
  • Confirm all five issue numbers are correctly referenced

AI disclosure

This PR was drafted with assistance from Claude (Anthropic). The
deprecation text and issue summaries were composed by the AI agent based
on maintainer direction. All content was reviewed before committing.

🤖 Generated with Claude Code

Base automatically changed from enh/superbuild-ep-optimization to main April 5, 2026 09:49
@hjmjohnson hjmjohnson force-pushed the fix/dwi-convert-sunset branch from d6d09ca to 9cd35f6 Compare April 5, 2026 10:20
hjmjohnson
hjmjohnson commented Apr 5, 2026
View reviewed changes

@hjmjohnson hjmjohnson left a comment •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This deprecation note is good, but please do attempt to resolve the issues reported before closing them.

All DWIConvert fixes should be part of this PR.

@hjmjohnson hjmjohnson changed the title DOC: Deprecate DWIConvert in favor of dcm2niix WIP: DOC: Deprecate DWIConvert in favor of dcm2niix Apr 5, 2026
@hjmjohnson hjmjohnson force-pushed the fix/dwi-convert-sunset branch from 9cd35f6 to bfed28e Compare April 5, 2026 14:13
@hjmjohnson hjmjohnson changed the title WIP: DOC: Deprecate DWIConvert in favor of dcm2niix BUG: Deprecate DWIConvert; fix #294, #370, #457; document #342, #412 Apr 5, 2026
@hjmjohnson

hjmjohnson commented Apr 5, 2026

Copy link
Copy Markdown
Member Author

Addressing the reviewer request to resolve issues before closing. Two commits now on this branch:

What was fixed

#294 (Generic gradient tags) — TryExtractSupp49DWIData() added to DWIDICOMConverterBase: reads DICOM Supplement 49 standard tags [0018,9087] / [0018,9089] from SharedFunctionalGroupsSequence (5200,9229) as a fallback. Toshiba and other Supplement 49-compliant scanners should now work without a dedicated converter class.

#370 (Segfault Nrrd→NIfTI, partial) — Added defensive check in NrrdToFSL.cxx: if GetNumberOfComponentsPerPixel() == 0 the function now returns EXIT_FAILURE with a clear error message instead of proceeding into a crash. Without a reproducer the full crash path could not be confirmed.

#457 (UINT16 as INT16, diagnostic) — PixelValueType = short is hardcoded throughout the pipeline; refactoring it is not viable for a deprecated tool. A runtime warning is now printed when PixelRepresentation=0 is detected so users know their data is being misread.

Root cause documented (not fixed)

#412 (Coronal DICOM) — SetDirectionsFromSliceOrder() assumes axial acquisition; fixing it requires ground-truth coronal DICOM test data not available in this repo.

#342 (NrrdToFSL LAS vs LPS) — ConvertBvecsToFromFSL() applies a j-axis flip for FSL convention; ConvertBetweenFileFormats does not. The discrepancy is documented in the README with the exact functions involved.

All explanations are in DWIConvert/README.md.

@hjmjohnson @claude
DOC: Add DWIConvert deprecation notice and link to dcm2niix
979dc01 
DWIConvert is superseded by dcm2niix which has broader community
support and more active maintenance. This notice documents the
deprecation, lists known issues that will not be fixed, and
provides migration guidance.

Addresses #457, #412, #370, #342, #294

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hjmjohnson hjmjohnson force-pushed the fix/dwi-convert-sunset branch from bfed28e to 9a08a64 Compare April 5, 2026 15:17
hjmjohnson and others added 4 commits April 5, 2026 10:22
@hjmjohnson @claude
BUG: Add Supplement 49 gradient tag fallback to DWIDICOMConverterBase (…
1fcfe44 
…fixes #294)

DICOM Supplement 49 tags [0018,9087] (DiffusionBValue) and [0018,9089]
(DiffusionGradientOrientation) inside SharedFunctionalGroupsSequence
(5200,9229) were only read by HitachiDWIConverter.  Toshiba and other
vendors that follow the standard were silently producing empty gradient
tables.

Add TryExtractSupp49DWIData() as a protected method on
DWIDICOMConverterBase so that any vendor converter can call it as a
fallback when its own vendor-specific tags are absent.  The method
returns false (leaving m_BValues / m_DiffusionVectors unchanged) if
the standard tags are absent, allowing callers to try the next path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@hjmjohnson @claude
BUG: Warn on unsigned DICOM pixel data read as signed short (#457)
13c03ec 
DWIConvert hardcodes PixelValueType = short (signed 16-bit) throughout
the entire pipeline.  DICOM files with PixelRepresentation=0 (unsigned
16-bit) silently produce incorrect intensity values for voxels > 32767,
since the unsigned value is reinterpreted as a negative signed integer.

A full fix would require refactoring the PixelValueType throughout the
entire class hierarchy -- not viable for a deprecated tool.  Emit a
runtime warning via std::cerr when PixelRepresentation=0 is detected in
LoadDicomDirectory() so users are aware rather than receiving silently
wrong data.  Tag 0028,0103 is read with throwException=false so the
absence of the tag (some vendors omit it) defaults to signed (1).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@hjmjohnson @claude
BUG: Guard NrrdToFSL against zero-component input (#370)
a14fa3f 
NrrdToFSL called GetNumberOfComponentsPerPixel() without checking that
the value is non-zero.  An Nrrd file missing the 4th (diffusion)
dimension, or one with corrupt metadata, could produce numComponents=0.
With the prior code this would:
  - proceed into a vacuous pixel-copy loop (silent no-op)
  - pass a zero-length vecLength to downstream metadata recovery
  - potentially crash in RecoverBVectors or WriteBValues

Add an early-exit with a diagnostic message before CreateVolume() is
called.  Also use the local numComponents variable for vecLength rather
than calling GetNumberOfComponentsPerPixel() a second time.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@hjmjohnson @claude
DOC: Document DWIConvert issue resolutions in README
0fd30f6 
Update DWIConvert/README.md to replace the bare 'will not be fixed'
list with per-issue entries covering:

  #294 (fixed)  -- TryExtractSupp49DWIData added; Supplement 49 tags
                    now supported as a fallback in all vendor converters
  #370 (partial) -- zero-component guard added in NrrdToFSL.cxx
  #457 (diag)   -- runtime warning on unsigned PixelRepresentation
  #412 (doc)    -- root cause: axial-only assumption in
                   SetDirectionsFromSliceOrder(); requires coronal
                   test data to fix safely
  #342 (doc)    -- root cause: ConvertBvecsToFromFSL() j-axis flip
                   discrepancy vs ConvertBetweenFileFormats; code
                   change risks breaking existing workflows

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@hjmjohnson hjmjohnson force-pushed the fix/dwi-convert-sunset branch from 9a08a64 to 0fd30f6 Compare April 5, 2026 15:24
@hjmjohnson hjmjohnson marked this pull request as ready for review April 5, 2026 15:33
@hjmjohnson hjmjohnson merged commit 26affe1 into main Apr 5, 2026
2 checks passed
@hjmjohnson hjmjohnson deleted the fix/dwi-convert-sunset branch April 5, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

1 participant

@hjmjohnson