neurodatascience / nipoppy

Lightweight framework for neuroimaging-clinical data organization/processing
https://nipoppy.readthedocs.io/en/latest/
MIT License
10 stars 19 forks source link

[ENH] Add default tracker config files for fMRIPrep and FreeSurfer, and update default invocation files #265

Closed michellewang closed 2 weeks ago

michellewang commented 3 weeks ago

Summary:

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.64%. Comparing base (5f107e0) to head (51051bb).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #265 +/- ## ======================================= Coverage 98.64% 98.64% ======================================= Files 28 28 Lines 1766 1766 ======================================= Hits 1742 1742 Misses 24 24 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

michellewang commented 2 weeks ago

@nikhil153 what should we populate output_spaces with?

michellewang commented 2 weeks ago

@nikhil153 I'm thinking of renaming the descriptors:

Mostly because I'm looking at BIDSCOIN now and it has the bidsmapper, bidseditor and bidscoiner CLIs, and I'm wondering if it would make more sense to just name the descriptors based on the actual command name.

However, I still think the invocations should probably still have the step name in them, so it might be confusing to have e.g. the descriptor called dcm2bids_helper-3.1.0.json and the invocation called dcm2bids-3.1.0-prepare.json?

Do you have any thoughts/preference?

nikhil153 commented 2 weeks ago

@nikhil153 what should we populate output_spaces with?

The default one, which i think is this: MNI152NLin2009cAsym. The reason we should explicitly add this into invocation is better visibility. Often people forget what space / resolution they use or how to change it before launching large number of jobs. This would help, hopefully!

nikhil153 commented 2 weeks ago

@nikhil153 I'm thinking of renaming the descriptors:

  • dcm2bids-3.1.0-convert -> dcm2bids-3.1.0.json
  • dcm2bids-3.1.0-prepare -> dcm2bids_helper-3.1.0.json

Mostly because I'm looking at BIDSCOIN now and it has the bidsmapper, bidseditor and bidscoiner CLIs, and I'm wondering if it would make more sense to just name the descriptors based on the actual command name.

However, I still think the invocations should probably still have the step name in them, so it might be confusing to have e.g. the descriptor called dcm2bids_helper-3.1.0.json and the invocation called dcm2bids-3.1.0-prepare.json?

Do you have any thoughts/preference?

I think mapping descriptors 1-1 on the tools CLI makes sense. It also detaches them from nipoppy terms which is consistent with what we discussed last time. As for the invocations, I think it's fine to name them differently since now we are in nipoppy scope where we try to abstract out / standardize run calls. From user perspective neither the passengers nor crew members will ever see descriptors, so it should be fine! :-)

michellewang commented 2 weeks ago

The default one, which i think is this: MNI152NLin2009cAsym.

Okay, I'll add that. In the tracker I have * instead of the specific space entities -- I assume that is still fine? This way even if users change the output spaces then the tracker would still work out-of-the-box.