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.

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

[FIX] Add non-zero floor to T2* values #585

Merged
merged 2 commits into from
Aug 2, 2020
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jul 15, 2020

Closes #513.

Todo:

  • Write test

Changes proposed in this pull request:

  • Add a new function to determine smallest non-zero T2* value that will not resolve to zero in optimal combination step. Replace values in the T2* maps > 0 and < this value with the value.
  • Add a new unit test for the function. This test doesn't improve coverage, but is important.

@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #585 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
+ Coverage   93.44%   93.53%   +0.09%     
==========================================
  Files          26       26              
  Lines        1953     1965      +12     
==========================================
+ Hits         1825     1838      +13     
+ Misses        128      127       -1     
Impacted Files Coverage Δ
tedana/decay.py 95.48% <100.00%> (+1.27%) ⬆️

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 ddaa4d5...dbfb63c. Read the comment docs.

@tsalo tsalo requested review from eurunuela and jbteves July 15, 2020 19:25
Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding this fix @tsalo !

@tsalo tsalo requested a review from dowdlelt August 1, 2020 13:57
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.

This looks good to me, that error is always concerning to see. In the 'appearance is everything' sense, fixing this is a huge deal. I'm approving, but a thought about optionally (verbosely?) logging the number of voxels that needed to be fixed could be useful. Lets call that a future request.

@tsalo tsalo merged commit d933592 into ME-ICA:master Aug 2, 2020
@tsalo tsalo deleted the fix-513 branch August 2, 2020 18:00
@jbteves
Copy link
Collaborator

jbteves commented Aug 2, 2020

@dowdlelt would you mind writing up an issue for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change WIll make a non-trivial change to outputs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Divide by zero error when using explicit mask
3 participants