[DOC, FIX, RF] Fix typos in documentation and refactor fit_decay/fit_decay_ts. #49
Conversation
Refactor Fit
Rather than manually inputting the dimensionality of the input data, check automatically. Also, I originally made a mistake in checking the shape of the data. I guess data should be inputted in 3D form (SxExT) rather than 5D (XxYxZxExT)
tsalo
changed the title
|
I should have held off on merging @TomMaullin's PR, but I forgot about this one, so both sets of changes (the doc fix and the fit_decay refactor) have been mushed together in one PR. Sorry about that! |
|
Nice ! Is this ready for review ? |
|
Yeah, it's ready. |
| t2saf_ts[:, :, :, vol] = t2saf | ||
| s0vaf_ts[:, :, :, vol] = s0vaf | ||
|
|
||
| [t2sa, s0va, _, _, t2saf, s0vaf] = fit_decay( |
There was a problem hiding this comment.
What do you think about converting this to, instead of calling fit_decay inside of fit_decay_ts, being able to throw a timeseries=True flag ?
There was a problem hiding this comment.
@TomMaullin if you'd like to address this in a different PR, I can merge this one and leave #25 open !
There was a problem hiding this comment.
Personally, I think it would be good to leave that to a separate PR! This ones looks good to me 😄
There was a problem hiding this comment.
Hi @emdupre , @rmarkello
Sorry for the delayed response! Would you still like me to address this or has it now been taken care of?
There was a problem hiding this comment.
Sorry for my delayed response ! I went ahead and merged this PR, but #25 is still open with the idea of eventually refactoring so that we could pass a timeseries=True argument or something similar. If you're interested in taking that on, we'd love to have your contribution !!
|
Thanks ! I do have one specific question about how we're refactoring |
|
And I added a merge conflict from #41 😓 |
# Conflicts: # tedana/model/monoexponential.py
Took the wrong merge commit !
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
+ Coverage 23.02% 23.37% +0.34%
==========================================
Files 23 23
Lines 1355 1335 -20
==========================================
Hits 312 312
+ Misses 1043 1023 -20
Continue to review full report at Codecov.
|
|
This is passing, despite the coverage warning ! Opened #53 to fix this for future PRs. |
# Conflicts: # tedana/model/monoexponential.py
|
🎉 it passes! Sorry for the headache of merge conflicts caused by integrating #41... These updates look great! The docs are coming along handily 👍 |
|
LGTM ! Merging ! @TomMaullin I'll keep #25 open, too 😄 |
After the build succeeded, I noticed a couple of typos. Namely:
nibabelwere not workingtedana.workflowsdescription was incorrectnp.ndarrayinstead ofnumpy.ndarray, breaking intersphinx links