[ENH] Update Figure Generation Code #236
Conversation
Creates a pie chart showing 1) The total variance explained by each
component in the outer ring, 2) the variance explained by each
individual component in the inner ring, 3) counts of each classification
and 4) the amount of unexplained variance.
|
@tsalo just noticed that I've duplicated your work - I looked through your viz_update branch and it seems like I wasn't too far off the mark on my fixes but would still appreciate a close look. |
| comp_rho = "{0:.2f}".format(comptable.iloc[compnum][2]) | ||
| plt_title = 'Comp. {}: variance: {}%, kappa: {}, rho: {}'.format(compnum, comp_var, | ||
| comp_kappa, comp_rho) | ||
| comp_var = "{0:.2f}".format(comptable.iloc[compnum]["variance explained"]) |
There was a problem hiding this comment.
Should we use "variance explained" or "normalized variance explained" here and elsewhere?
@emdupre Thoughts?
There was a problem hiding this comment.
Thinking more on this, both of these sum up to be close to 100, even in cases where the PCs entered into in the ICA step explain <100% of the variance, right, because ICs are not orthogonal? I'm looking at an older run of the meica tedana, where the comp_table says 76.7% of the variance was explained, but summing the Variance and Variance norm columns gives me 100.03 and 96.78% respectively.
Means that 100 - sum(variance_expl) that calculates the unexplained variance is never going to work quite right.
Nor would calculating the variance explained (as is done in io.py, around line 202) and adding that in give the right number - because then the pie chart would be relative to the total (ie ~100 variance from ICA + unexplained = some_number >100). Perhaps some scaling would be needed for the values, prior to giving them to that part of figure creation.
There was a problem hiding this comment.
👍 for postponing the discussion until #223 !
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
- Coverage 47.19% 46.34% -0.85%
==========================================
Files 32 32
Lines 1977 2026 +49
==========================================
+ Hits 933 939 +6
- Misses 1044 1087 +43
Continue to review full report at Codecov.
|
Co-Authored-By: dowdlelt <logan.dowdle@gmail.com>
I did play around a bit in via-update, but went a little too far with refactoring your code, which is why I don't plan to open a PR or anything. It did help me understand what was happening though. |
|
Travis really doesn't like 3 long lines, so I'll think on that. Otherwise, I've dealt with everything but the trailing semicolons. |
|
This now seems to work great without complaints. Pending further review and general acceptance of this pull request, I am curious if we would want to save the variance explained question for another round... |
|
It looks great to me. I'm happy to hold off on discussing variance explained. In fact, my concerns regarding the variance explained calculated within |
|
I've been meaning to dig into #223 more - guess we all have a bit more time with the call delayed. Thanks for all the help with this! |
| comp_rho = "{0:.2f}".format(comptable.iloc[compnum][2]) | ||
| plt_title = 'Comp. {}: variance: {}%, kappa: {}, rho: {}'.format(compnum, comp_var, | ||
| comp_kappa, comp_rho) | ||
| comp_var = "{0:.2f}".format(comptable.iloc[compnum]["variance explained"]) |
There was a problem hiding this comment.
👍 for postponing the discussion until #223 !
Co-Authored-By: dowdlelt <logan.dowdle@gmail.com>
Co-Authored-By: dowdlelt <logan.dowdle@gmail.com>
Co-Authored-By: dowdlelt <logan.dowdle@gmail.com>
|
Awesome Awesome. Thanks for all the feedback folks |
|
Since everyone's happy with the current version, are we ready to merge? |
|
Push the button whenever you're ready, @tsalo 👍 |
Closes #232 .
Changes proposed in this pull request:
Example of pie chart:
Example of new timeseries plots, with titles.
