♻️ Refactor DICOMWSIReader

[shaneahmed]

• This is the first PR to start refactoring WSIReader into smaller files for maintainability
• Restructures wsireader and creates a dicom file for DICOMWSIReader
• Updates is_dicom to use WsiDicom.open to detect a DICOM file.
• Fixes Cannot visualize DICOM WSI series: wsidicom.errors.WsiDicomNotFoundError: Level files not found in provided files #942 using is_dicom to detect the container instead of .dcm files inside the container folder. This should fix tiatoolbox visualize for dicom files.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (8d13267) to head (f0b8333).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1087   +/-   ##
========================================
  Coverage    99.90%   99.90%           
========================================
  Files           93       94    +1     
  Lines        11911    11922   +11     
  Branches      1562     1562           
========================================
+ Hits         11900    11911   +11     
  Misses           5        5           
  Partials         6        6           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[shaneahmed]

@fedorov Please can you check if this PR resolves #942 ?

[fedorov]

@shaneahmed I am sorry I have not been able to get to it yet. I do want to test. Please let me know how urgent this is.

There were some other DICOM-related issues identified while preparing tiatoolbox tutorials for IDC https://github.com/ImagingDataCommons/idc-tiatoolbox#known-issues-and-workarounds-tiatoolbox-160, so I wanted to take this opportunity to upgrade to the latest version and revisit those.

[shaneahmed]

@shaneahmed I am sorry I have not been able to get to it yet. I do want to test. Please let me know how urgent this is.

There were some other DICOM-related issues identified while preparing tiatoolbox tutorials for IDC https://github.com/ImagingDataCommons/idc-tiatoolbox#known-issues-and-workarounds-tiatoolbox-160, so I wanted to take this opportunity to upgrade to the latest version and revisit those.

@fedorov This is only refactoring PR as we are restructuring WSIReader. As we introduce WSI format detection, this should resolve #942. Once dicom is refactored, we will restructure rest of the readers as well.

Re known issues in version 1.6.0, a lot have changed in v2.0.0. We have completely restructured the engines. Please can you try with version 2 and raise any issues on tiatoolbox GitHub?

[fedorov]

Please can you try with version 2 and raise any issues on tiatoolbox GitHub?

Yes, exactly my plan, just have not got to it yet... how much time do I have before you plan to merge?

[shaneahmed]

Please can you try with version 2 and raise any issues on tiatoolbox GitHub?

Yes, exactly my plan, just have not got to it yet... how much time do I have before you plan to merge?

From my side, this is ready to merge, once you have confirmed that the issue is resolved.

[fedorov]

I iterated with Claude, and it identified these issues, which seem to be legit.

All three points below are about the PR's proposed code on top of today's develop — none involve
1.6.0. Current develop still has the old suffix-sniffing is_dicom
(detection.py:38-43).
The surrounding dispatch (verify_supported_wsi / try_dicom) is current develop code that the PR
leaves alone; its factory.py diff is import-only.

1. is_dicom discards the opened WsiDicom

detection.py:40-45
calls WsiDicom.open(path) and drops the result on the floor:

try:
    WsiDicom.open(path)
except WsiDicomNotFoundError:
    return False
else:
    return True

Measured on a 4-instance IDC series (wsidicom 0.34.0, CPython 3.12): file handles return to 0 after
each call, because refcounting finalizes the discarded object immediately — so this is not a
persistent fd leak. Holding the references instead gives 4 fds per open, and close() releases them.
Still worth with WsiDicom.open(path): — WsiDicom implements __enter__/__exit__ but has no
__del__, so correctness here relies entirely on CPython refcount timing rather than on anything in
the class.

2. Up to 3 WsiDicom.open calls per WSIReader.open

The probe is not on a cold path — it runs twice before the reader is even constructed:

step	code
1	factory.py:55 verify_supported_wsi(...) → :84-86 is_dicom(input_path)
2	factory.py:220 try_dicom → is_dicom(input_path) again
3	factory.py:221 → dicom.py:36 self.wsi = WsiDicom.open(input_img)

Measured cost per successful open on a local 4-instance series: 11-14 ms, i.e. ~25 ms of pure
waste per slide open. Cost is dominated by parsing the instance metadata, so it grows with series size
and would be materially worse over network-backed storage (s3/gcs paths, which open_wsi explicitly
supports via UPath). Failed opens on non-DICOM inputs are cheap (0.1-0.6 ms for a .svs), so the two
wasted probes every non-DICOM file now pays are negligible — the DICOM path is the one that hurts.
try_dicom is also first in the
_handle_special_cases
chain, so nothing short-circuits it. Passing the already-opened handle through (or memoizing the probe)
would collapse this to one open.

3. except WsiDicomNotFoundError is narrow, and it changes the error users see

Every input tried raised exactly that class, so no escape was observed in practice — but the clause is
the only guard, and any other failure from WsiDicom.open (unsupported transfer syntax, remote
filesystem errors) will now propagate out of WSIReader.open instead of returning False and letting
the chain continue.

Second-order effect: .dcm is not in the suffix allow-list at
factory.py:91-106,
so DICOM inputs depend entirely on is_dicom returning True. Anything wsidicom can't open now dies at
factory.py:107-109
with File ... is not a supported file format. Verified: a nonexistent .dcm path and a .dcm file
containing garbage bytes both return False and surface that message, where previously they reached
DICOMWSIReader.__init__ and raised a wsidicom error naming the real problem. A mistyped slide path
reporting "not a supported file format" is a meaningful debuggability regression — catching
WsiDicomError broadly while letting FileNotFoundError through would keep the win without it.

[shaneahmed]

2. Up to 3 WsiDicom.open calls per WSIReader.open

Thanks @fedorov

1. is_dicom discards the opened WsiDicom & 2. Up to 3 WsiDicom.open calls per WSIReader.open are related and not just limited to dicom. We will fix this once all the readers have been refactored. We plan to update the base class and fix this issue for all the readers.

[shaneahmed]

3. except WsiDicomNotFoundError is narrow, and it changes the error users see

Re 3. except WsiDicomNotFoundError is narrow, and it changes the error users see

there are two issues raised. except WsiDicomNotFoundError is narrow and .dcm is not in the suffix allow-list at.

I will look into except WsiDicomNotFoundError is narrow

Re .dcm is not in the suffix allow-list at, this is intended as the new design is not dependent on suffix but the file itself. If the file is valid, it should read it irrespective of the extension.