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, DOC] Use countnoise in decision table within selcomps #238

Merged
merged 5 commits into from
Mar 22, 2019

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Mar 20, 2019

Closes None. In the process of documenting selcomps, I realized that I must have made a mistake when I added in table support to selcomps. Namely, the variable countnoise was not being incorporated properly in the decision tables used for certain component selection steps. countnoise was used properly in other steps, so I think the impact is minimal.

Changes proposed in this pull request:

  • Remove countnoise variable from selcomps. When we switched over to using tables, I forgot to remove this variable, so it was being initialized as an array of zeros but wasn't filled out (because the associated column of comptable was). countnoise was only used in the decision table steps, so it was essentially being ignored as a metric there.
  • Add some documentation about the different steps of the component selection decision tree. The goal here is not just to determine how each metric is used, but also why that's the best way to evaluate what that metric is meant to evaluate.
  • Split rationale I002 (Rho greater than Kappa or more significant voxels in S0 model than R2 model) into I002 (Rho greater than Kappa) and I003 (more significant voxels in S0 model than R2 model), and shift all other rationales accordingly. While this step was originally consolidated, there is an "or" in the step, so there's no reason to keep these two rationales together.
  • Update candartA and candartB rationales. Both are considered kinds of mid-Kappa, so I changed the rationales from "Artifact candidate type [A|B]" to "Mid-Kappa artifact type [A|B]".
  • Replace anomalous ct_df variable in tedpca with comptable to match package convention.

Also, add more documentation and update rationales.
- Rejection candarts are types of mid-Kappa
- I002 can be split into I002 (Rho>Kappa) and I003 (more significant
voxels in S0 model than R2 model)
@tsalo tsalo requested a review from emdupre March 20, 2019 14:28
@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #238 into master will decrease coverage by 0.11%.
The diff coverage is 1.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
- Coverage   46.34%   46.23%   -0.12%     
==========================================
  Files          32       32              
  Lines        2026     2031       +5     
==========================================
  Hits          939      939              
- Misses       1087     1092       +5
Impacted Files Coverage Δ
tedana/selection/select_comps.py 5.03% <0%> (-0.15%) ⬇️
tedana/decomposition/eigendecomp.py 10.52% <2.22%> (-0.07%) ⬇️

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 e39d1f0...6a69892. Read the comment docs.

Copy link
Member

@emdupre emdupre left a comment

Choose a reason for hiding this comment

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

This is great ! I made two small comments just on clarity, since I think that's my driving motivation around selcomps overall. Feel free to merge when you're ready 👍

tedana/selection/select_comps.py Outdated Show resolved Hide resolved
tedana/selection/select_comps.py Outdated Show resolved Hide resolved
- acc_poss —> acc_prov
- Remove extraneous lines
@tsalo tsalo merged commit 8b12fc4 into ME-ICA:master Mar 22, 2019
@tsalo tsalo deleted the doc-selcomps branch March 23, 2019 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants