Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[REF] Reorganize fit_decay #468

Merged
merged 9 commits into from
Nov 18, 2019
Merged

[REF] Reorganize fit_decay #468

merged 9 commits into from
Nov 18, 2019

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Nov 17, 2019 •
edited
Loading

Closes #440 and closes #439.

Changes proposed in this pull request:

  • Split the actual decay-fitting parts of decay.fit_decay into fit_loglinear and fit_monoexponential.
  • Only fit curve to voxels that will be used in the limited or full S0/T2* maps. This should speed up curve-fitting with large number of echoes.
  • Remove ascending T2*/S0 estimates from fit_decay outputs.
  • Write out adaptive_mask.nii.gz by default.

tsalo added 4 commits November 17, 2019 14:12
@tsalo
Refactor fit_decay.
b9a4459
@tsalo
Some more cleanup.
10875eb
@tsalo
Slight refactor.
e0dba99 
NOTE: Now I know why the maps were different! In this refactor, I’m
cropping values in T2* limited and T2* full after masking, while before
we did it before. So any voxel with an adaptive mask value of 1 has a
value of 0 in the old T2* limited, but 1 in the new.
@tsalo
Fix the cropping.
d7f9448
@tsalo tsalo added the T2*/S0 estimation issues related to the estimation of T2* and S0 label Nov 17, 2019
@tsalo tsalo requested a review from dowdlelt November 17, 2019 20:28
@codecov
Copy link

codecov bot commented Nov 17, 2019 •
edited
Loading

Codecov Report

Merging #468 into master will decrease coverage by 0.48%.
The diff coverage is 79.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
- Coverage   82.22%   81.74%   -0.49%     
==========================================
  Files          39       39              
  Lines        2594     2629      +35     
==========================================
+ Hits         2133     2149      +16     
- Misses        461      480      +19
Impacted Files Coverage Δ
tedana/workflows/t2smap.py 67.08% <ø> (ø) ⬆️
tedana/tests/test_decay.py 100% <100%> (ø) ⬆️
tedana/workflows/tedana.py 65.72% <100%> (-0.41%) ⬇️
tedana/decay.py 77.68% <75.51%> (-11.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 444fb28...c1e033a. Read the comment docs.

dowdlelt
dowdlelt requested changes Nov 18, 2019
View reviewed changes
Copy link
Collaborator

@dowdlelt dowdlelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks great - definitely gets cleaner, and should make it much easier to add new methods in the future - though at the moment I'm just thinking about the biexponential.

tedana/decay.py Outdated Show resolved Hide resolved
tedana/tests/test_decay.py Outdated Show resolved Hide resolved
tedana/tests/test_decay.py Outdated Show resolved Hide resolved
tedana/tests/test_decay.py Outdated Show resolved Hide resolved
tedana/tests/test_decay.py Outdated Show resolved Hide resolved
tedana/tests/test_decay.py Outdated Show resolved Hide resolved
dowdlelt
dowdlelt approved these changes Nov 18, 2019
View reviewed changes
Copy link
Collaborator

@dowdlelt dowdlelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is good to merge. No other issues jumping out to me

@tsalo tsalo merged commit ce72cf1 into ME-ICA:master Nov 18, 2019
@tsalo tsalo deleted the enh/improve-curvefit branch November 18, 2019 15:59
@tsalo tsalo mentioned this pull request Nov 19, 2019
Merged
@effigies effigies mentioned this pull request Nov 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dowdlelt dowdlelt dowdlelt approved these changes

Assignees
No one assigned
Labels
T2*/S0 estimation issues related to the estimation of T2* and S0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorporate bounds into curvefit T2*/S0 estimation Drop ascending T2*/S0 estimates
2 participants
@tsalo @dowdlelt