Added inital xnat_to_bids function and error checking for it by slimnsour · Pull Request #317 · TIGRLab/datman

slimnsour · 2021-11-16T17:22:06Z

This is my base work for getting xnat_to_bids working on our system. This adds the function xnat_to_bids that pulls the data directly from xnat into a tmp folder, and then dcm2bids is run on that tmp folder with an appropriate json for that study. This is called instead of the usual xnat to nii pull if you call dm_xnat_extract.py with the tag --use-dcm2bids. Several arguments for dcm2bids are also allowed to be controlled (like --dcm-config, --forceDcm2niix, and --clobber).

I would greatly appreciate feedback on this! Right now I have some design decisions that I'm unsure of as well:

The default dcm2bids json location for each study is at /archive/data/$STUDY/metadata/dcm2bids.json. I think this might be a good idea instead of forcing it into the yamls for each of the studies, since the jsons mapping each study could overwhelm the file contents. Instead, we can have a separate location (its own repo or a folder or in datman-config) for all the jsons and then soft link them to metadata/dcm2bids.json. An example folder is already in our bids_dev repo.
The default bids folder is at /archive/data/$STUDY/data/bids. I believe this makes sense, especially with the option --bids-out that allows you to override it for testing purposes.
Dcm2Bids is currently being imported as a package and being called as a function. I did this as a configurable option since our Dcm2Bids is already a python package, but this would require datman to depend on Dcm2Bids now. I'm open to other options, namely using a singularity container. I already have the container with the newest dcm2niix and our dcm2bids installed, it's just a matter of switching it out if we want to use that instead.

I still have other features I might add in the future (fixing fmap json intendedFor's, defacing), but I'd like to get the base functionality out there to get your guys' opinions on it.

welcome · 2021-11-16T17:22:08Z

Thanks for opening this pull request! We have detected this is the first time for you to contribute to datman. Please check out our contributing guidelines.
We invite you to list yourself as a datman contributor, so if your name is not already mentioned, please modify the .zenodo.json file and insert your data last in the contributors list. Example:

{
   "name": "Contributor, New datman",
   "affiliation": "Department of na na na datman'ing, Open Science Made-Up University",
   "orcid": "<your id>",
   "type": "Researcher"
}

Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed.

jerdra

Good work putting this together @slimnsour! Just have some comments on code-styling and 1 bigger ask to simplify the CLI interface. Lmk if you have questions :)

jerdra · 2021-11-17T14:10:06Z

+    --use-dcm2bids               Pull xnat data and convert to bids using dcm2bids
+    --keep-dcm               Keep raw dcm pulled from xnat in temp folder
+    --dcm-config CONFIG      Path to dcm2bids config file
+    --bids-out BIDS_OUT      Path to output bids folder
+    --forceDcm2niix          Force dcm2niix to be rerun in dcm2bids
+    --clobber                Clobber previous bids data


With all the new bids-specific CLI options would it make sense to perhaps switch over to argparse with a sub-parser to avoid over-crowding w/options that are specific to particular circumstances?

Added the CLI in the new commits! Thanks for the suggestion

jerdra · 2021-11-17T14:11:40Z

+                        keep_dcm=keep_dcm,
+                        dcm2bids_config=dcm2bids_config,
+                        bids_out=bids_out,
+                        forceDcm2niix=forceDcm2niix,
+                        clobber=clobber)


I'd refactor this and bundle these parameters as a dcm2bids Config object/dataclass

I second Jer's suggestion here

Added dataclass in the new commits, great suggestion!

jerdra · 2021-11-17T14:15:11Z

+        if (use_dcm2bids):
+            # Ensure the config is available
+            if dcm2bids_config is None:
+                try:
+                    dcm2bids_config = datman.utils.locate_metadata("dcm2bids.json", config=cfg)
+                except datman.scanid.ParseException:
+                    logger.error("No config file available for study {}."
+                                 "".format(study))
+            # Ensure the bids output is available
+            if bids_out is None:
+                bids_out = cfg.get_path("bids")
+            xnat_to_bids(xnat, 
+                        project, 
+                        experiment,
+                        keep_dcm=keep_dcm,
+                        dcm2bids_config=dcm2bids_config,
+                        bids_out=bids_out,
+                        forceDcm2niix=forceDcm2niix,
+                        clobber=clobber)
+        else:
+            process_experiment(xnat, project, experiment)


I'm even more convinced that we should use a sub-parser now! You could set up subparser.set_defaults(function=<target_func>) to simplify the logic:

https://stackoverflow.com/questions/49038616/argparse-subparsers-with-functions

Added the new argparse parser in the new commits but no subparser, see #317 (comment)

jerdra · 2021-11-17T14:33:22Z

I would greatly appreciate feedback on this! Right now I have some design decisions that I'm unsure of as well:

The default dcm2bids json location for each study is at /archive/data/$STUDY/metadata/dcm2bids.json. I think this might be a good idea instead of forcing it into the yamls for each of the studies, since the jsons mapping each study could overwhelm the file contents. Instead, we can have a separate location (its own repo or a folder or in datman-config) for all the jsons and then soft link them to metadata/dcm2bids.json. An example folder is already in our bids_dev repo.

Agreed, I think this is a good solution

The default bids folder is at /archive/data/$STUDY/data/bids. I believe this makes sense, especially with the option --bids-out that allows you to override it for testing purposes.

Perf! This is how bidsify.py currently works as well

Dcm2Bids is currently being imported as a package and being called as a function. I did this as a configurable option since our Dcm2Bids is already a python package, but this would require datman to depend on Dcm2Bids now. I'm open to other options, namely using a singularity container. I already have the container with the newest dcm2niix and our dcm2bids installed, it's just a matter of switching it out if we want to use that instead.

Maybe we could have datman have a datman[bids] setup.cfg option so that it becomes an optional dependency? This would require some refactoring (i.e moving dcm2bids import into a function, perhaps the one called by a subparser if that's what we end up going for)

DESm1th

This looks good Salim, thank you! It looks like the 'making symlinks for the legacy datman folders' part was abandoned though. Without it a lot of our bin scripts and stuff will break at the moment so we might have to hold off on running nightly with the dcm2bids flag for a while

DESm1th · 2021-11-17T16:27:06Z

+                        keep_dcm=keep_dcm,
+                        dcm2bids_config=dcm2bids_config,
+                        bids_out=bids_out,
+                        forceDcm2niix=forceDcm2niix,
+                        clobber=clobber)


I second Jer's suggestion here

…y comments, specified error catching

pep8speaks · 2021-11-18T20:43:33Z

Hello @slimnsour, Thank you for updating!

In the file bin/dm_xnat_extract.py:

Line 220:9: E128 continuation line under-indented for visual indent
Line 220:9: E125 continuation line with same indent as next logical line

To test for issues locally, pip install flake8 and then run flake8 datman.

Comment last updated at 2021-11-19 21:48:25 UTC

jerdra · 2021-11-19T14:58:23Z

@slimnsour maybe instead of a sub-parser we could use ArgumentParser.add_argument_group like @josephmje did in #318?

It would achieve separation of bids options from base options to mitigate overcrowding as we had originally wanted

…d over to argparse for organized argument groups

slimnsour · 2021-11-19T17:23:43Z

@slimnsour maybe instead of a sub-parser we could use ArgumentParser.add_argument_group like @josephmje did in #318?

It would achieve separation of bids options from base options to mitigate overcrowding as we had originally wanted

Great suggestion! I've added that to the newest commit.

For context, the subparsers in argparse (from my testing) requires that the option to select the subparser be the very first positional argument choose between the subparsers. So, if we wanted to have subparsers to separate between extracting using our usual datman way and dcm2bids, we would have to call it like dm_xnat_extract.py {datman, dcm2bids} <study> ..., which is fine, but it would break our current management scripts that assume <study> to be the first argument. If anyone can figure out a good way to do this then I would happy to implement it but it might be out of the scope of this PR.

Here's the new output of dm_xnat_extract.py --help using argparse:

usage: dm_xnat_extract.py [-h] [--blacklist FILE] [--server URL] [-u USER]
                          [--dont-update-dashboard] [-t [tag,...]]
                          [--use-dcm2bids] [--bids-out DIR]
                          [--dcm-config FILE] [--keep-dcm] [--force-dcm2niix]
                          [--clobber] [-d] [-q] [-v] [-n]
                          study [experiment]

Extracts data from XNAT archive folders into a few well-known formats.

optional arguments:
  -h, --help            show this help message and exit

Options for choosing data from XNAT to extract:
  study                 Nickname of the study to process
  experiment            Full ID of the experiment to process (default: None)
  --blacklist FILE      Table listing series to ignore override the default
                        metadata/blacklist.csv (default: None)
  --server URL          XNAT server to connect to, overrides the server
                        defined in the configuration files. (default: None)
  -u USER, --username USER
                        XNAT username. If specified then the environment
                        variables (or any credential files) are ignored and
                        you are prompted for a password. Note that if multiple
                        servers are configured for a study the login used
                        should be valid for all servers.. (default: None)
  --dont-update-dashboard
                        Dont update the dashboard database (default: False)
  -t [tag,...], --tag [tag,...]
                        List of scan tags to download (default: None)

Options for using dcm2bids:
  --use-dcm2bids        Pull xnat data and convert to bids using dcm2bids
                        (default: False)
  --bids-out DIR        Path to output bids folder (default: None)
  --dcm-config FILE     Path to dcm2bids config file (default: None)
  --keep-dcm            Keep raw dcm pulled from xnat in temp folder (default:
                        False)
  --force-dcm2niix      Force dcm2niix to be rerun in dcm2bids (default:
                        False)
  --clobber             Clobber previous bids data (default: False)

Options for logging and debugging:
  -d, --debug           Show debug messages (default: False)
  -q, --quiet           Minimal logging (default: False)
  -v, --verbose         Maximal logging (default: False)
  -n, --dry-run         Do nothing (default: False)

josephmje · 2021-11-19T17:37:02Z

+    g_dcm2bids.add_argument(
+        "--use-dcm2bids", action="store_true", default=False,
+        help="Pull xnat data and convert to bids using dcm2bids"


Should the --use-dcm2bids option exist outside of this? And then, only if that option is used, the g_dcm2bids options are exposed.

I moved it to the main group cause it makes more sense there like you said, but Im not sure how to only expose dcm2bids arguments when we read from the parser with argparse. Reading from a specific group is not supported and subparsers would break the usage I mentioned in #317 (comment)

i think @josephmje wants to warn/prevent users from using BIDS related flags when --use-dcm2bids isn't selected. What we could do is to add a bit of code after parsing to check if any of the BIDS config exists when --dcm2bids is not selected and either:

Throw up a warning indicated that those args will be ignored

Use ArgumentParser.error with a message

I prefer option 2 personally as it'll prevent mistakes from running through

Yup! Could do something like

if not args.use_dcm2bids and (args.bids_out or args.dcm_config or ...): parser.error("dcm2bids configuration requires --use-dcm2bids")

Ah gotcha, thank you guys for laying that out! I added it in b1cb96b

Co-authored-by: Michael Joseph <josephmje.22@gmail.com>

…and switched to os.path instead of pathlib

jerdra

Good work and thanks for incorporating our feedback @slimnsour!

Not relevant anymore

Added inital xnat_to_bids function and error checking for it

ec85f2a

slimnsour requested review from DESm1th, benselby, edickie, jerdra and josephmje November 16, 2021 17:22

auto-assign bot requested a review from jskocic November 16, 2021 17:22

jerdra requested changes Nov 17, 2021

View reviewed changes

DESm1th previously requested changes Nov 17, 2021

View reviewed changes

josephmje reviewed Nov 17, 2021

View reviewed changes

Comment thread bin/dm_xnat_extract.py Outdated

jerdra mentioned this pull request Nov 18, 2021

[ENH] Implement XNAT Extract DAG TIGRLab/air-tigrs#11

Open

4 tasks

Added dcm2bids options dataclass for xnat_to_bids, removed unnecessar…

7bb514f

…y comments, specified error catching

Moved run function out of dcm2bids dataclass, fixed pep8 issues, move…

f7486cb

…d over to argparse for organized argument groups

josephmje reviewed Nov 19, 2021

View reviewed changes

slimnsour and others added 3 commits November 19, 2021 13:04

Apply suggestions from @josephmje code review

879cb38

Co-authored-by: Michael Joseph <josephmje.22@gmail.com>

Removed unnecessary docopt info, moved --use-dcm2bids to main group, …

c130cee

…and switched to os.path instead of pathlib

Added parser check if using dcm2bids commands incorrectly

b1cb96b

jerdra approved these changes Nov 22, 2021

View reviewed changes

josephmje approved these changes Nov 22, 2021

View reviewed changes

DESm1th self-requested a review November 30, 2021 17:38

DESm1th merged commit e3d8ada into TIGRLab:master Nov 30, 2021

Conversation

slimnsour commented Nov 16, 2021

Uh oh!

welcome bot commented Nov 16, 2021

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!

Choose a reason for hiding this comment

Uh oh!

slimnsour Nov 19, 2021 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jerdra commented Nov 17, 2021

Uh oh!

DESm1th left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pep8speaks commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-11-19 21:48:25 UTC

Uh oh!

jerdra commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slimnsour commented Nov 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerdra Nov 19, 2021 • 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!

josephmje Nov 19, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

jerdra left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jerdra left a comment •

edited

Loading

slimnsour Nov 19, 2021 •

edited

Loading

pep8speaks commented Nov 18, 2021 •

edited

Loading

jerdra commented Nov 19, 2021 •

edited

Loading

jerdra Nov 19, 2021 •

edited

Loading

josephmje Nov 19, 2021 •

edited

Loading