StrongResearch / dimble

Nimble Digital Imaging IO for Medicine
BSD 3-Clause "New" or "Revised" License
7 stars 2 forks source link

Make DicomValue a homogeneous collection instead of heterogeneous #13

Open shepmaster opened 1 year ago

shepmaster commented 1 year ago

The previous type allowed for a collection of mixed integers / strings / floats, etc. Since the DICOM format doesn't allow for that, we can change the type to enforce a single inner type.

We also create our own Value enum instead of using rmpv::Value. This allows us to ignore non-UTF8 strings and integers that cannot be represented as i64.

StrongChris commented 1 year ago

Looks awesome, thanks for the PR.

I've run the tests and some errors are occuring.

FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[OBXXXX1A_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[JPGLosslessP14SV1_1s_1f_8b.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[gdcm-US-ALOKA-16.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[OBXXXX1A.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[OBXXXX1A_expb.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[OBXXXX1A_rle_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[OBXXXX1A_expb_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[liver_expb.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[OBXXXX1A_rle.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[gdcm-US-ALOKA-16_big.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[liver.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dicom_to_dimble[eCT_Supplemental.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[OBXXXX1A_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[JPGLosslessP14SV1_1s_1f_8b.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[gdcm-US-ALOKA-16.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[OBXXXX1A.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[OBXXXX1A_expb.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[OBXXXX1A_rle_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[OBXXXX1A_expb_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[liver_expb.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[OBXXXX1A_rle.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[gdcm-US-ALOKA-16_big.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[liver.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_load_dimble[eCT_Supplemental.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[OBXXXX1A_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[JPGLosslessP14SV1_1s_1f_8b.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[gdcm-US-ALOKA-16.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[OBXXXX1A.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[OBXXXX1A_expb.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[OBXXXX1A_rle_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[OBXXXX1A_expb_2frame.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[liver_expb.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[OBXXXX1A_rle.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[gdcm-US-ALOKA-16_big.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[liver.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON
FAILED test/test_dicom_e2e.py::test_dimble_to_dicom[eCT_Supplemental.dcm] - dimble_rs.DimbleError: Could not parse the DICOM JSON

36/195 are failing so seeing what is common among those should make fixing this easy. Merging this PR is not a massive priority at the moment, so I'll debug this one when I get a chance, possibly next week.

StrongChris commented 1 year ago

This is what each of the tracebacks look like.

json_path = PosixPath('/tmp/OBXXXX1A.ir.json'), pixel_path = PosixPath('/tmp/OBXXXX1A.ir.safetensors'), output_path = PosixPath('/tmp/OBXXXX1A.dimble')

    def _ir_to_dimble(json_path: Path, pixel_path: Path, output_path: Path) -> None:
>       dimble_rs.dicom_json_to_dimble(str(json_path), str(output_path), str(pixel_path))
E       dimble_rs.DimbleError: Could not parse the DICOM JSON
E       
E       Caused by this error:
E         1: data did not match any variant of untagged enum DicomValue at line 336 column 10

.venv/lib/python3.10/site-packages/dimble/dimble.py:61: DimbleError

Error messages look very nice :)

shepmaster commented 1 year ago

Very odd. Are those tests that are not run via make test? I do not have any test errors when running that locally.

pytest --benchmark-disable
============================================================================================================================ test session starts =============================================================================================================================
platform darwin -- Python 3.11.3, pytest-7.3.1, pluggy-1.0.0
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/shep/Projects/integer32/strong-research/dimble
configfile: pyproject.toml
testpaths: test
plugins: benchmark-4.0.0
collected 231 items

test/test_dicom_e2e.py ........................................................................................................................................................................                                                                        [ 72%]
test/test_dtypes.py .........                                                                                                                                                                                                                                          [ 76%]
test/test_nifti_e2e.py ...................................................                                                                                                                                                                                             [ 98%]
test/test_sq.py ...                                                                                                                                                                                                                                                    [100%]

============================================================================================================================== warnings summary ==============================================================================================================================
test/test_dicom_e2e.py::test_dimble_to_dicom[gdcm-US-ALOKA-16.dcm]
test/test_dicom_e2e.py::test_dimble_to_dicom[gdcm-US-ALOKA-16_big.dcm]
  /Users/shep/Projects/integer32/strong-research/dimble/lib/python3.11/site-packages/pydicom/valuerep.py:290: UserWarning: Invalid value for VR CS: 'ABDOM/RAD'. Please see <https://dicom.nema.org/medical/dicom/current/output/html/part05.html#table_6.2-1> for allowed values for each VR.
    warnings.warn(msg)

test/test_dicom_e2e.py::test_dimble_to_dicom[JPEG2000_UNC.dcm]
  /Users/shep/Projects/integer32/strong-research/dimble/lib/python3.11/site-packages/dimble/dimble.py:111: RuntimeWarning: invalid value encountered in cast
    ds.compress(RLELossless, pixel_data.astype(np.uint16))

test/test_dicom_e2e.py::test_dimble_to_dicom[color-px.dcm]
test/test_dicom_e2e.py::test_dimble_to_dicom[color-pl.dcm]
  /Users/shep/Projects/integer32/strong-research/dimble/lib/python3.11/site-packages/pydicom/valuerep.py:290: UserWarning: Invalid value for VR TM: '11:20:00'.
    warnings.warn(msg)

test/test_dicom_e2e.py::test_dimble_to_dicom[color-px.dcm]
test/test_dicom_e2e.py::test_dimble_to_dicom[color-pl.dcm]
  /Users/shep/Projects/integer32/strong-research/dimble/lib/python3.11/site-packages/pydicom/valuerep.py:290: UserWarning: Invalid value for VR DA: '1994.11.05'.
    warnings.warn(msg)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
====================================================================================================================== 231 passed, 7 warnings in 25.10s ======================================================================================================================
shepmaster commented 1 year ago

Ah! I have to run make install-dev by hand before running the tests, it's not tracked as part of the makefile dependencies.

shepmaster commented 1 year ago

OK, should be fixed now. I got confused and thought that both Alphabetic and SeqField could only occur once, but that's not true for SeqField