-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add initial support for the DICOM format #21385
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
base: dev
Are you sure you want to change the base?
Conversation
Co-authored-by: Björn Grüning <bjoern@gruenings.eu>
|
Package tests are failing https://github.com/galaxyproject/galaxy/actions/runs/19894695277/job/57021998725?pr=21385#step:7:5453 because metadata is not set as expected. I assume that this is because pydicom is not installed for the package tests (in this case, -- |
|
Package tests still failing, not sure why:
|
You can ignore that, it's another issue that will be fixed by #21384 . |
I don't understand this. https://github.com/galaxyproject/galaxy/actions/runs/19934924298/job/57156962812?pr=21385#step:8:3697 ✅ ✅ def test_tiff_sniff():
for filename in (
...,
"1.tiff", # <--!!!
...,
):
fname = get_test_fname(filename)
...
assert not OMETiff().sniff(fname), f"filename: {filename}" # <--!!!
assert Tiff().sniff(fname), f"filename: {filename}" # <--!!!Why does ❌ Note: I think this must be caused by something changed in https://github.com/galaxyproject/galaxy/pull/21385/files/df5fe20cf57b5084163645d4e63179bcc57e17a8..d8375ee948434b2600cefebc719301e0d4666c46, because the unit tests passed before that. |
| buf = io.BytesIO(file_prefix.contents_header_bytes) | ||
| with tifffile.TiffFile(buf) as tif: | ||
| return tif.is_ome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling this won't work for larger files where the image is larger than the contents_header_bytes .
Maybe you can use the @disable_parent_class_sniffing decorator from lib/galaxy/datatypes/sniff.py and restore the previous sniff method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it (see kostrykin#4), but when I put the @disable_parent_class_sniffing decorator on the OMETiff class, the sniff method does not get called. The decorator seems to disable sniffing.
The test/unit/data/datatypes/test_images.py::test_ome_tiff_sniff fails with
> assert OMETiff().sniff(fname)
E AssertionError: assert False
E + where False = <lambda>('lib/galaxy/datatypes/test/1.ome.tiff')
E + where <lambda> = <galaxy.datatypes.images.OMETiff object at 0x7f99946c3190>.sniff
E + where <galaxy.datatypes.images.OMETiff object at 0x7f99946c3190> = OMETiff()
Despite that OMETiff.sniff is implemented as
def sniff(self, filename: str) -> bool:
raise ValueError('OMETiff.sniff called') # <-- !!! this error is *not* reported !!!
with tifffile.TiffFile(filename) as tif:
return tif.is_omethus, OMETiff.sniff is not called when the @disable_parent_class_sniffing decorator is in place.
Here is the error from running the test: https://github.com/kostrykin/galaxy/actions/runs/19942920690/job/57185121231?pr=4#step:8:11238
That said, I think the current solution should work, because tifffile.TiffFile only reads the headers/metadata of the file. The pixel content only is read when the corresponding methods are used (they are not used here).
On the other hand, since we do not know how large metadata can be, or how it is organized, if there is a possibility to get this working without sniff_prefix, I'd go for that.
|
I think all remaining CI failures are unrelated to this PR. |
This PR builds on the work initiated by @hexylena in #18895 (the PR was abandoned).
DICOM is a popular file format in medical imaging.
DICOM files are identified using the pydicom package, which is also used to read metadata. The pydicom package is a pure Python package that has no further dependencies and is 2.4 MB in size.
How to test the changes?
(Select all options that apply)
License