New DICOM study import script

[maximemulder]

Description

This PR is a rewrite in Python (+ cleaning up / refactor) of the DICOM archiving scripts in Python, with a few added features.

In more details, the changes are the following:

• dicom-archive/dicomTar.pl → python/scripts/import_dicom_study.py
• dicom-archive/dicomSummary.pl → python/scripts/summarize_dicom_study.py
• dicom-archive/DICOM/DCMSUM.pm (and a bunch of other code) → python/lib/import_dicom_study/*

This PR aims to be feature-compatible with the current Perl scripts, such that it can eventually replace it seemlessly. It removes some arguments that were unused (-centerName, -today and -addMriUpload), sorts the DICOM archive in a year subdirectory automatically (this was previously done later in the pipeline), and automatically use the tarchive directory as a target.

This PR also adds the --session argument to determine the DICOM archive session based on the Python configuration file (and fail if no session is found).

Testing

Tested all options successfully with Raisinbread and MPN data. The data looks identical with both the old DICOM archiver and this new script.

Potential future work

• Replace the old DICOM archiver by the new script within the existing MRI pipeline.
• Clean up the tarchive table.
• Add integration test for CI.

Notes for existing projects

• need to install pydicom library pip install pydicom

[maximemulder]

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:

• I did not use our custom argument parser because there is now argparse in the standard Python library, which handles all the cases we want. We probably want to transition to that long term.
• I also wrote a few database helpers for my code, but I think in the long term we should transition to a database and query abstraction library, like sqlalchemy which I've heard about.

[maximemulder]

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.

[cmadjar]

@maximemulder I read through the code and have a few comments/refactoring suggestions. Let me know your thoughts.

Note: I did not test yet, just went through the code.

[maximemulder]

Thanks for the review @cmadjar ! I wrote some comments on a few points. Points with no comments mean that I agree or will look into it.

[cmadjar]

@maximemulder I think if you rebase this one, it will pass the flake8 now that your PR with the python install update has been merged. Thanks!

Also, regarding the -mri_upload_update, this option is used by projects that do not use the imaging uploader to upload their scan and run directly dicomTar.pl. I think we should still support that functionality as some projects might still be using the old way of importing DICOMs. Otherwise, the script to create the MINC or BIDS dataset would break because there is no entry in the mri_upload table. Actually, before you do this change, could you bring it up during the next imaging call? It should be next week on August 8th. Thank you :)

[maximemulder]

Ok I replied to all your (very good !) comments @cmadjar. The only ones I have intentionally left out are these ones :

1. Move SQL queries to database_lib (waiting on a decision on SQLAlchemy at the next imaging meeting).
2. Add the --db-upload parameter (done for a while on my SQLAlchemy branch)
3. Use log.py (full implementation depends on --db-upload)

If you like the rest of the PR, I can add these very quickly after the next imaging meeting.

[laemtl]

Waiting for the first test implementation to polish it.

[cmadjar]

Why make the distinction between --insert and --update here? I think a simple --database option is enough.

If --overwrite is used, then, it would update, if not, it would insert after having check that there is not already an entry in the DB for the StudyUID.

[maximemulder]

I did this to provide more options in case there are inconsistencies between the file system and the database (a file but no entry or an entry but no file). --insert and --update are only about the database, while --overwrite is about the file system. I don't have a strong opinion on this but it seemed like a good idea when I did it.

[cmadjar]

ok. Interesting. I have to think about this one. I would have a tendency to have a simple database flag and then figure out if data should be updated or inserted in the database when want to update the database instead of having to do that outside of the script in order to properly call import_dicom_study.py... I will sleep on it and get back to you.

[maximemulder]

Hhhhm, not sure I understand your last message correctly. In any case, I am ok to have a single --update option that both overwrites the file system and updates the entries in the database. However, the script should definitely not overwrite / update anything if the user does not specify it IMO, that is too error prone and could lead to a loss of data IMO.

[cmadjar]

@maximemulder very nice work overall!!

Two summary points:

• issue with a Philips odd B1 images (discussion on slack with details about the issue found and suggestion of improvements)
• issue with enhanced DICOMs (I want to take a closer look at this one on Monday to see if it would be possible to read enhanced DICOMs parameters). That last one is not a blocker to merge the PR though and can be implemented later on.

[maximemulder]

Thanks for your feedback @cmadjar ! For your information, I tested the PR with some Rainsinbread data and some MPN data, which is acquired using a Siemens scanner, so testing this PR on DICOMs from other scanners is very valuable.

When I wrote the DICOM sorting logic of this PR, I tried to closely follow the code of DCMSUM.pl, which itself seems to follow the structure of the generated "DICOM summary" file rather closely. I took another look at the code I wrote (back in June damn) and the (demonic) Perl code in DCMSUM.pl.

The DCMSUM.pl file basically looks at all the DICOM files and considers that each DICOM series is uniquely identified by a tuple of sequence name, echo number and series number. It also considers that a DICOM series has a single echo time, repetition time, inversion time... This is obviously false, notably for multi-echo series (which I have on MPN). When that is the case, the DCMSUM.pl code only saves a single DICOM series in the database, with a single record of the parameters used (a single echo time out of all the echo times of the series, a single repetition time out of all the repetition times of the series...).

As far as I understand it, the standard way to uniquely identify a DICOM series within a DICOM study is to use the SeriesUID DICOM field. This field is nullable in our database schema, but I checked and there is no NULL entry in neither Rainsinbread nor C-BIG. I think fixing our database schema to handle multi-parameter series is out of scope for this PR. IMO, what we should do for now in this PR is:

• Stop assuming DICOM series can be identified using the tuple of sequence name, echo number and series number.
• Create a DICOM series entry in the database for each tuple of all the parameters of the series. This results in duplicate entries for a single DICOM series, but it is better than ignoring the some of the parameter values of the series IMO (also note that there can already be duplicate entries since the current identification method is flawed).
• Instead of inserting all the DICOM series in the database, and then querying the database to get the DICOM series of each DICOM file as it is currently done (copied from DCMSUM.pl), simply use a dict[DicomStudyDicomSeries, list[DicomStudyDicomFile]] when identifying the DICOM files, and insert each series along with its files, resulting in more straightforward and less error-prone code.

Pointers for DCMSUM.pl:

• Where the code gets data from the DICOM files: https://github.com/aces/Loris-MRI/blob/main/dicom-archive/DICOM/DCMSUM.pm#L808-L815
• Where the code goes through the DICOM files data to list the DICOM series: https://github.com/aces/Loris-MRI/blob/main/dicom-archive/DICOM/DCMSUM.pm#L657-L674

[cmadjar]

@maximemulder I test the python DICOM ingestion script on:

• 6 Philips datasets
• 4 Siemens XA 30 (enhanced DICOMs)
• 2 GE datasets

All differences noted between the perl and python ingestion were improvements (ability to read enhanced DICOMs, ability to distinguish multi-echo or multi-tr DICOMs etc...).

The only missing piece at the moment is that if the DeviceSerialNumber header has been removed from the DICOMs due to anonymization, I get the following error:

"import_dicom_study.py -p database_config_27.py -s /data/tmp/CHSTL0158_705052_V02_SessionA_1.3.12.2.1107.5.2.43.166158.30000025012107581494200000808_siemens_prisma --insert
Extracting DICOM information... (may take a long time)
Traceback (most recent call last):
  File "/opt/loris/bin/mri/python/scripts/import_dicom_study.py", line 314, in <module>
    main()
  File "/opt/loris/bin/mri/python/scripts/import_dicom_study.py", line 147, in main
    dicom_summary = get_dicom_study_summary(args.source, args.verbose)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/loris/bin/mri/python/lib/import_dicom_study/summary_get.py", line 39, in get_dicom_study_summary
    study_info = get_dicom_study_info(dicom)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/loris/bin/mri/python/lib/import_dicom_study/summary_get.py", line 83, in get_dicom_study_info
    read_value(dicom, 'DeviceSerialNumber'),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/loris/bin/mri/python/lib/import_dicom_study/summary_get.py", line 154, in read_value
    raise Exception(f"Expected DICOM tag '{tag}' but found none.")
Exception: Expected DICOM tag 'DeviceSerialNumber' but found none."

Ideally, when reading the DICOM, should either read the content of the DICOM tag if available or set it to NULL to avoid this error

[maximemulder]

@cmadjar Hhhhm, none of the scanner fields in the tarchive table are nullable, so I guess I will store None in the code, and insert an empty string in the database for now (we can still change the schema and insert NULL later). Should I do that for all the scanner fields (manufacturer, model, serial number, software version) or only some of them ?

[cmadjar]

@cmadjar Hhhhm, none of the scanner fields in the tarchive table are nullable, so I guess I will store None in the code, and insert an empty string in the database for now (we can still change the schema and insert NULL later). Should I do that for all the scanner fields (manufacturer, model, serial number, software version) or only some of them ?

Good catch! Looks like the current dicomTar.pl is indeed inserting with an empty string so probably best to use the same strategy here. And I think it is a good idea to apply that on all the Scanner fields, in case other anonymization software completely removes those.

Thanks!

[cmadjar]

Great job!!!