aces / Loris-MRI

The set of scripts that preprocess and insert MRI data into the database.
10 stars 50 forks source link

New dicom archive #1117

Open MaximeMulder opened 2 months ago

MaximeMulder commented 2 months ago

Description

This PR is a rewrite (and cleaning/modernization) of dicom-archive/dicomTar.pl, dicom-archive/dicomSummary.pl and dicom-archive/DICOM/DCMSUM.pm in Python, which intends to replace them in the long term. This PR does not replace them for now as we should do more testing and integrate these scripts with the rest of the pipeline before doing so, likely in the next LORIS version imo.

Presentation

This PR adds three Python scripts:

To support these scripts, the python/lib/dicom/ directory was added, which contains various modules used by the aforementioned scripts.

Breaking changes

I removed the following options from dicomTar.pl / dicom_archive.py, as they did not seem to be used by any other script:

I did not port the tools/addSeriesAndFileRecords.pl script as it seems to not be used in LORIS / LORIS-MRI, and should maybe be deleted in the long term. It is however very easy to re-code with the new architecture (it is currently already a simple script).

PLEASE TELL ME IF ANYONE HAS A PROBLEM WITH THESE CHANGES. Other suggestions are also welcome.

Remaining work

Before merging:

Longer term work:

MaximeMulder commented 1 month ago

Okay, I think the PR is finished, I am happy with the code and tested it on a few DICOMs on my side.

The tests are failing because I am using a few match statements, which were added in Python 3.10. So I'd like to bump the Python minimum supported version to 3.10, if not, I can remove them (but obviously prefer the newer and shinier version).

We should also test it on DICOMs from other projects. Then, when this PR is merged, I can make another PR to adopt it in the different superscripts, although a problem will be that this script requires the Python profile file, while the scripts that call dicomTar.pl use the Perl one (so both will probably need to be passed as arguments).

I will note a few things on the code:

MaximeMulder commented 2 days ago

Blocked on Python 3.10, as well as #1141 and #1142 to see if this PR should be updated with new libraries.

NOTE for myself: Bring a fix from the new-mri-upload branch before merging (I don't remember which one but it should be easy) and do not initiate the database connection if the profile is not supplied.