PennLINC / qsiprep

Preprocessing of diffusion MRI
http://qsiprep.readthedocs.io
BSD 3-Clause "New" or "Revised" License
140 stars 58 forks source link

Restructure CLI to bring in line with nipreps #697

Closed tsalo closed 2 months ago

tsalo commented 8 months ago

Summary

I have a few thoughts about how the QSIPrep CLI could be brought in line with the other preps a bit more.

  1. Split QSIRecon out of QSIPrep. It could still be in the same package, but with a separate command.
  2. Combine --recon-input and --freesurfer-input into a --derivatives that accepts a list of paths.
  3. Add --level parameter to preprocessing workflow.
    • I don't know what specifically this would entail- I imagine it's different for each modality, depending on what kind of preprocesing is involved, but the minimal option in fMRIPrep and ASLPrep involves outputting transform files, but not resampled imaging files.

This is related to #696.

Additional details

  1. Simpler command-line interfaces.
  2. Better synergy with other tools.
  3. Easier for users to mix and match pipelines if QSIPrep is treated as a preprocessing pipeline and QSIRecon is treated as a postprocessing recon (like XCP-D, fMRIPost-AROMA, fMRIPost-tedana, and fMRIPost-rapidtide).

Drawbacks

  1. The full combination of preprocessing and reconstruction would take two commands instead of one.
mattcieslak commented 8 months ago

This all looks good, except for --level, which isn't possible for qsiprep for now. The reason is that eddy and many of its advanced features can't be split up into transforms. For example, the outlier replacement and slice to volume motion correction can't be delayed until the end of the pipeline by simply keeping transform files around.

I think you're totally right about (1) too. They don't share very much code and having both of their commandline options intermingled makes use of either more confusing. It might even be a good chance to start qsirecon in a clean repo

cookpa commented 8 months ago

+1 on a standalone qsirecon with standalone documentation.

tsalo commented 8 months ago

Would it make more sense to completely excise QSIRecon from QSIPrep (i.e., new repo, new Docker image) or to keep it all in the same package, with a separate command-line interface? Having them separate could reduce the size of the packages and the containers, but it could also increase the maintenance burden.

mattcieslak commented 8 months ago

I've been thinking about this one - there isn't a ton of overlap between the two workflows, but there definitely is some. I have a hunch that almost all of the overlap could be replaced by things already in nipreps.

tsalo commented 5 months ago

To follow up from a convo on Slack: during the recent NiPreps refactor, @mattcieslak noticed that there was more shared code between the preprocessing and reconstruction workflows than originally expected. I'd like to build a comprehensive list of the shared code at some point, but at least for now it's probably not a good idea to move QSIRecon (or QSIPost, which I prefer) into a separate repo.

We could start by splitting the CLI at least.

mattcieslak commented 5 months ago

I'm on board - would you recommend forking qsiprep to start? Or starting with a fresh repo and copying as needed?

tsalo commented 5 months ago

I think we can just work within qsiprep for the initial CLI split. If we want to move forward with a full repo split, we can do that later.

tsalo commented 5 months ago

Then again... it's probably just easier to create a new repo and make QSIPrep a dependency of QSIRecon. Otherwise I have to create multiple versions of a bunch of files in the repo for the two pipelines.

mattcieslak commented 5 months ago

Going through the workflows now, the overlap isn't as big as I was initially worried about. We might not even need a dependency on a qsiprep

mattcieslak commented 5 months ago

the config object will also benefit from having 2 separate packages. it's currently not as straightforward as a config for a package that does only one main thing

tsalo commented 5 months ago

Okay, I've got the QSIPost repo started up in pennlinc/qsipost.