search

ME-ICA / tedana

TE-dependent analysis of multi-echo fMRI
https://tedana.readthedocs.io
GNU Lesser General Public License v2.1
157 stars 94 forks source link

Reduce duplication between t2smap and tedana workflows #488

Open tsalo opened 4 years ago

tsalo commented 4 years ago

Summary

tedana_workflow is the focus of most of our efforts, even though t2smap_workflow is also probably frequently used (via fMRIPrep). As a result, t2smap_workflow keeps becoming more and more out-of-date with respect to our current goals. For example, it still uses the label argument instead of an out_dir one. While we need to maintain compatibility with fMRIPrep, we should still work to keep t2smap_workflow current.

Given that the contents of t2smap_workflow overlap completely with a portion of tedana_workflow, we should convert t2smap_workflow into a function that can be called both by the t2smap CLI and by tedana_workflow.

Next Steps

  1. Update tedana.workflows.t2smap_workflow with respect to changes that have been made to tedana.workflows.tedana_workflow.
  2. Move the CLI portion of tedana.workflows.t2smap_workflow into a separate file. Have that CLI argparser call the updated function in tedana.workflows.t2smap_workflow.
    • The CLI can still have different restrictions than the CLI for the tedana workflow (e.g., FIT is supported for t2smap but not for tedana).
  3. Replace the T2*/S0 estimation and optimal combination sections of tedana.workflows.t2smap_workflow with a call to tedana.workflows.t2smap_workflow.
tsalo commented 4 years ago

Alternatively, what if we removed the t2smap workflow altogether and just added a --no-denoise argument to the tedana workflow that, if set, would stop after writing out the optimally combined data and T2*/S0 maps? We'd need to allow FIT in the tedana workflow, but we could make it dependent on --no-denoise.

emdupre commented 4 years ago

I think I'd actually prefer the first way -- where tedana_workflow calls t2smap_workflow. Mostly because the tedana workflow is already quite long, and I do think they're conceptually slightly different pieces (i.e., fMRIPrep is unlikely to call the full tedana workflow any time soon).

Is there a strong reason to go with the second approach ?

tsalo commented 4 years ago

Once I started working on the first approach I realized that there would still be a lot of unnecessary duplication and opportunities for one method to end up outdated compared to the other.

I don't think they're conceptually different. One is just the first part of the other. Some options that are fine for just T2* estimation and optimal combination are incompatible with TE-dependent denoising, but that's not really unique to this particular division of functionality.

emdupre commented 4 years ago

Once I started working on the first approach I realized that there would still be a lot of unnecessary duplication and opportunities for one method to end up outdated compared to the other.

Could you expand a little bit on this ? That's clearly the situation we're in now (so addressing it is definitely something we should be doing !), but I'm not sure how we'd end up with outdated methods if we're calling t2smap inside the tedana workflow.

tsalo commented 4 years ago

If we move the core of t2smap into a separate function that is called by both the t2smap CLI and tedana workflow, then that function will likely include the following steps:

  1. Load data and check echo times
  2. Generate adaptive mask
  3. Estimate T2*/S0
  4. Write out T2*/S0 files
  5. Perform optimal combination
  6. Write out optimal combination file
  7. Return results in array format

The function will require a large number of parameters, and it will need to return catd, ref_img, tes, mask, masksum, t2s_limited, t2s_full, tsoc, maybe s0_limited (not sure), and maybe s0_full (also not sure). One problem I have with that is that we may need to frequently update both the inputs and outputs to this function based on what we need later on in the denoising steps. That and the number of parameters and outputs would make it difficult to read.

The new t2smap CLI code would need to set up and clean up the loggers and reports, and, depending on how much duplication we want with input checking between the t2smap and tedana workflow functions, do a lot of that as well. That's where t2smap could become outdated again.

jbteves commented 4 years ago

During the conference call today we'd discussed breaking this discussion about workflows into a separate, more general issue. Is that okay with you @tsalo @emdupre ?