[ENH] Refactor dm_xnat_extract + allow nii folders to be replace with sym links to bids folders by DESm1th · Pull Request #334 · TIGRLab/datman

DESm1th · 2022-08-19T21:54:27Z

Ok this is a really big one (sorry guys), but most of it is just reshuffled code. The really important bits are the contents of datman/exporters.py which contains a set of classes to export to different formats. All the old export_x_command functions from xnat_extract are now classes in that file, as is the contents of the xnat_to_bids function, and I've pulled all the database updating code (which was spread through xnat_extract) into its own exporter. I've added a NiiLinkExporter here that takes a subject's bids folder and makes datman-style links for it in the nii folder.

Within xnat_extract itself the most important bit is the export_scans function which does the work of creating the exporters, downloading any raw data needed to run them, and then running them.

The changes in datman/xnat is code I've moved from dm_xnat_extract, and the changes elsewhere were small fixes for bugs I encountered while refactoring.

- Added a blacklist check before matching dm names to bids names because apparently blacklisted items are also removed from the bids folders. - Fixed references to cfg, glob function, and ident method

TODO: - Remove the need for use_bids in BidsOptions class - Finish testing edge cases - Add unit tests

nii links werent regenerating when one was missing, unless at least one exporter that needed raw data was found. This has been fixed.

- NiiExporter was failing if run twice, this is fixed. - DBExporter was outputting an error message when json side cars werent found, even if the entire series only existed on xnat

…ions exist

If outputs existed and another exporter that didnt need raw data needed to run, BidsExporter would mistakenly try to run again and report an error due to an empty temp dir (no input data).

josephmje · 2022-09-01T13:22:57Z

+                         f"Reason - {type(exc).__name__} {exc} ")
+
+
+class NrrdExporter(SeriesExporter):


I think we can get rid of the nrrd and mnc exporters. We're no longer using CIVET, which requires the mnc files. And our tractography pipelines have a nifti to nrrd converter built in.

Fair enough! I was just trying not to drop functionality but I can purge those classes :)

DESm1th · 2022-09-19T20:51:06Z

Is it alright for me to merge this?

jerdra · 2022-09-20T13:15:28Z

@DESm1th sorry this dropped off my radar! I'll review the last bits today after our meet!

jerdra

Thanks for the massive refactor @DESm1th ! Nothing in my review should block a merge, more just questions for clarity since there're some bits where I can't fully understand the intent immediately!

jerdra · 2022-09-20T18:06:28Z

+                bids_niftis.append(os.path.join(path, basename))
+        return bids_niftis
+
+    def match_dm_to_bids(self, dm_names, bids_names):


Is this ever meant to be called outside the object? Would dm_names and bids_names ever be provided outside of using self?

Likely not, but I think I left it unprotected since I ended up calling it a lot while testing that the matching worked as expected. I can change this

I'll leave that up to you! My preference would be to remove the extra arguments for clarity 🙈

jerdra · 2022-09-20T18:26:30Z

+        return False
+
+    def export(self, *args, **kwargs):
+        # Re run this before exporting, in case new BIDS files exist.


A little bit confused about why this needs to be re-initialized. Is it expected that the state of the BIDS directory may change between initializing this class and calling its export function? Doesn't it kind of make the state variables of the class useless/misleading?

This was the result of an edge case when the data is exported for the very first time. The class would be initialized, the names would be mapped to an empty bids folder (so no nii files) and then when the exporters actually ran, on the first run through no links would be produced.

You're right that it might make more sense to remove the state variables for this reason, but I think there was a good reason why I didnt at the time. I'll have to look at it more closely again when I fix the issues you pointed out to try to remember why I kept them

Oh ok i took a second look at dm_xnat_extract and understand what you mean here now re: the edge case. I think you end up using the initialized state to check if the outputs exist right? It looks like you might still need it in that case!

DESm1th added 30 commits May 2, 2022 20:03

[FIX] Make sure session is added to dashboard when only dcm_to_bids used

24561a4

[FIX] Ensure resources get exported if only using dcm_to_bids

77f75df

[ENH] Ensure datman links get made when dcm2bids is used.

806be0f

Merge remote-tracking branch 'upstream/master' into link_nii

4686358

[FIX] Prevent duplicate files from being found, fix var name

e5ceb97

[FIX] Stop exceptions when experiment not found on xnat

47e5d8a

[FIX] Stop exceptions when a site doesnt configure XnatArchive

a14d721

[FIX] Typos and other small bugs

1db36e7

- Added a blacklist check before matching dm names to bids names because apparently blacklisted items are also removed from the bids folders. - Fixed references to cfg, glob function, and ident method

[FIX] Bad var name, mismatches when bids folder missing an early run

513c6fb

[FIX] Make sure bids names retain full path, and correct types used

6c8e093

Ensure fmaps get linked to nii folder

711f366

Refactored code a bit for readability + to reduce duplication

250eda8

Started moving exporters to separate module

4b628fc

Move some dm_xnat_extract functionality to datman/xnat.py

e3d008a

Update datman.scan path setting method, and add bids path

e4b469e

Add function to search for files in diff format directories

98a5f01

Allow datman.scan.Scan to take a string or an Identifier

2cccc60

Rough versions of exporters. Need to thoroughly test + refactor

11d20ce

Got the main exporters working (need to clean script)

d1f1ec3

Fixed issues with exporting dual echo series

87d29eb

Fixed bug when Identifier provided to init, and allow bids override

a8b8d8c

Refactored dm_xnat_extract (needs debugging)

40b82cf

Fixed bugs in most used exporters

0eecf54

Added docstrings to exporters + fixed style issues.

626ca59

Refactored dm_xnat_extract

e19218e

TODO: - Remove the need for use_bids in BidsOptions class - Finish testing edge cases - Add unit tests

Added ExporterException for datman/exporters to use

ea9bc60

Fixed small typos / syntax bugs

f8a3917

[FIX] Broken string because f string was incorrectly split

c0ecd6f

Fixed edge case causing exporters to not re-run

4070a4a

nii links werent regenerating when one was missing, unless at least one exporter that needed raw data was found. This has been fixed.

Fixed outputs_exist for DBExporter + addressed some edge cases

67efeb8

- NiiExporter was failing if run twice, this is fixed. - DBExporter was outputting an error message when json side cars werent found, even if the entire series only existed on xnat

DESm1th added 11 commits August 18, 2022 16:53

Ensure dry_run flag is passed into make_session_exporters

379c360

Update Scan class for bids + correct resources dir when multiple sess…

5bdbb19

…ions exist

Small refactor to is_dicom to ensure no exceptions are raised.

02e7c22

Updated BidsOpts so it's not created when user didnt specify bids

a6bd7da

Updated datman/xnat.py to handle downloading + applying datman names

f27e171

Fixed syntax issues + added some doc strings

b062b63

Added a plain resource_path attribute to datman.scan.Scan

a7c9bd1

Fixed small bugs and typos

655087c

Stop dcm2bids from logging an error when outputs exist

1cce1bc

If outputs existed and another exporter that didnt need raw data needed to run, BidsExporter would mistakenly try to run again and report an error due to an empty temp dir (no input data).

Removed dcm_config.json related code (now handled in BidsOptions)

5e41efe

Fixed up old dcm export code

62bba88

DESm1th requested review from jerdra, josephmje and salimansourc August 19, 2022 21:54

auto-assign bot requested a review from benselby August 19, 2022 21:54

josephmje reviewed Sep 1, 2022

View reviewed changes

[REF] drop the mnc + nrrd exporters as per Mike's comment

74ca3bd

jerdra approved these changes Sep 20, 2022

View reviewed changes

DESm1th merged commit d0b1953 into TIGRLab:main Sep 26, 2022

DESm1th deleted the link_nii branch September 26, 2022 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Refactor dm_xnat_extract + allow nii folders to be replace with sym links to bids folders#334

[ENH] Refactor dm_xnat_extract + allow nii folders to be replace with sym links to bids folders#334
DESm1th merged 42 commits intoTIGRLab:mainfrom
DESm1th:link_nii

DESm1th commented Aug 19, 2022

Uh oh!

josephmje Sep 1, 2022

Uh oh!

DESm1th Sep 1, 2022

Uh oh!

DESm1th commented Sep 19, 2022

Uh oh!

jerdra commented Sep 20, 2022

Uh oh!

jerdra left a comment •

edited

Loading

Uh oh!

Uh oh!

jerdra Sep 20, 2022

Uh oh!

DESm1th Sep 20, 2022

Uh oh!

jerdra Sep 20, 2022

Uh oh!

Uh oh!

jerdra Sep 20, 2022

Uh oh!

DESm1th Sep 20, 2022

Uh oh!

jerdra Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		f"Reason - {type(exc).__name__} {exc} ")


		class NrrdExporter(SeriesExporter):

Conversation

DESm1th commented Aug 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DESm1th commented Sep 19, 2022

Uh oh!

jerdra commented Sep 20, 2022

Uh oh!

jerdra left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jerdra left a comment •

edited

Loading