Refactor t2smap

[emdupre]

In #23, a fit function was added to allow for estimation of a T2*/S0 time series. This shares a lot of code with the original t2sadmap function, since it's a bit monolithic in its current implementation. It would be great to refactor these two functions so we have less duplicated code !

[TomMaullin]

Hi @emdupre , I am a Mozsprint participant and would be happy to give this a go! Do you have any tests or example data I could use to check I haven't affected the functionality of the code?

[emdupre]

That would be fantastic, @TomMaullin ! ☀️

Unfortunately we don't yet have unit tests, here, but that would be another great addition !! There is continuous integration on the repository, which is slower (as it runs the whole code base) but will confirm if your changes affect the outputs. Please let me know if I can answer any other questions !

[tsalo]

I don't think that the CI will catch errors in the fit method though, right? I don't think it's actually tested by anything at the moment.

[emdupre]

Oh, sorry, I missed your question about example data ! The file is too big to store on github, so we're grabbing it off Dropbox here.

[tsalo]

Related to this, now that there's a submodule called tedana.model.fit, I think having the FIT method named tedana.model.t2smap.fit is a little confusing. I only used FIT because that's how it's referred to in the Power multi-echo motion paper, but it's not the most informative name. Are there any good alternatives?

[emdupre]

You're totally write about testing fit @tsalo ! Good catch. No, we don't have any code to test that.

And I'm totally open to suggestions on names ! Maybe tedana.model.t2smap.fit_ts ?

[tsalo]

What about naming the two methods similarly? I'm not a huge fan of the names t2sadmap and t2smap, both because they're not very Pythonic and because I'm still not sure what t2sadmap means (t2smap is a bit clearer), but whatever it is, the FIT method can just be that plus _ts (e.g., ts2admap_ts).

What do you think about fit_decay or fit_me_decay and fit_decay_ts/fit_me_decay_ts? Or, if brevity's not important, fit_monoexponential_decay and fit_monoexponential_decay_ts?

[emdupre]

I completely agree about t2sadmap being unclear— I believe it's t2s_adaptive_map, but updating the names would be great.

I like the fit_decay and fit_decay_ts options ! I worry having "me" stand for both "monoexponential" and "multi-echo" could be somewhat confusing, and I do think some brevity would be appreciated on imports 😄

[TomMaullin]

I have run out of time but did give this a go and have submitted my WIP as a PR on tsalo/tedana in case any of it can be used. I'm afraid I haven't managed to test it as the docker container took me a while to download and run.

[emdupre]

Thanks so much for your efforts on this, @TomMaullin ! Sorry that the docker container took so long to get going, but the code looks great 🎉

[TomMaullin]

Hi @emdupre ,

I'm happy to address the timeseries flag issue if you still need!

That said... I am having some trouble running Tedana and t2sadmap in order to check my code isn't changing any output. I've cloned the Tedana repository, run python3 setup.py install and then tried to run the following in the command line using the data in the tests folder (which I thought would work based on this documentation):

tedana -d echo1.nii.gz echo2.nii.gz echo3.nii.gz -e 14.5 38.5 62.5

This gave the following error message:

Traceback (most recent call last):
  File "/usr/local/bin/tedana", line 11, in <module>
    load_entry_point('tedana==0.0.0', 'console_scripts', 'tedana')()
  File "/usr/local/lib/python3.5/dist-packages/tedana-0.0.0-py3.5.egg/tedana/cli/run_tedana.py", line 175, in main
    workflows.tedana.main(**vars(options))
AttributeError: 'function' object has no attribute 'main'

I must admit I don't usually work with this data - perhaps I have misunderstood something?

[tsalo]

I think I know what's going on. The scripts in cli need to be fixed now that the tedana workflow is called through tedana.workflows.tedana instead of tedana.workflows.tedana.main. I can open a PR to fix it.

[tsalo]

Hopefully #64 will fix the problem.

[emdupre]

Thanks @tsalo ! I think you're likely right— I'll review but am planning to merge as soon as Circle is passing.

Thanks for looking at this again @TomMaullin ! Having someone using the code is a huge help to figure out bugs like these 🐛

[tsalo]

As part of this, there is a discrepancy between different versions of the decay model fitting code (as discussed in #61) that needs to be resolved.

Namely, in the original tedana code, there is no start_echo argument, but the code loops through range(1, Ne+1), whereas in the original t2sadmap function it loops through range(2, Ne+1). In tedana.monoexponential.t2smap, start_echo is allowed and it loops through range(start_echo, Ne+1), but the tedana workflow uses start_echo=1, while the t2smap workflow uses start_echo=2.

[tsalo]

Given that the start_echo thing has been standardized across workflows as of #90 and that there isn't much duplication between fit_decay and fit_decay_ts now, is this issue resolved?

[emdupre]

That seems reasonable, @tsalo ! Closing now.