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

[ENH] Adds a scree plot showing metric values and variance sorted by Kappa. #432

Merged
merged 12 commits into from
Dec 13, 2019

Conversation

dowdlelt
Copy link
Collaborator

@dowdlelt dowdlelt commented Nov 8, 2019

Closes # None.

Purpose of this pull request is to generate a figure that shows the kappa and rho metric values (organized by kappa) as well as the variance explained by each component. This isn't a major figure, but it was included in some original evaluations. It also may highlight interesting things about variance and kappa values.

As an example, consider that component # 9 or so has very high variance (its pretty much a linear drift term) - but note that it also has kappa > rho.

Changes proposed in this pull request:

  • Add a new figure showing the 'kappa elbow'

image

@dowdlelt dowdlelt added enhancement issues describing possible enhancements to the project hackathon Issues to tackle in the NIH hackathon labels Nov 8, 2019
@dowdlelt dowdlelt changed the title [ENH] Adds a scree plot showing metric values and variance sorted by Kappa. [WIP, ENH] Adds a scree plot showing metric values and variance sorted by Kappa. Nov 8, 2019
@dowdlelt dowdlelt changed the title [WIP, ENH] Adds a scree plot showing metric values and variance sorted by Kappa. [ENH] Adds a scree plot showing metric values and variance sorted by Kappa. Nov 12, 2019
@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #432 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #432      +/-   ##
==========================================
+ Coverage   84.57%   84.64%   +0.07%     
==========================================
  Files          41       41              
  Lines        2910     2925      +15     
==========================================
+ Hits         2461     2476      +15     
  Misses        449      449
Impacted Files Coverage Δ
tedana/viz.py 93.41% <100%> (+0.55%) ⬆️
tedana/workflows/tedana.py 70.96% <100%> (+0.23%) ⬆️

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 a0cc6dc...e4c59e7. Read the comment docs.

@emdupre
Copy link
Member

emdupre commented Nov 12, 2019

@dowdlelt , @javiergcas is working on a little report that includes very similar scatterplots -- can you add a screenshot here, Javier ? 😸

@dowdlelt
Copy link
Collaborator Author

I saw some chatter on mattermost about that - excited to see it! Would be far more useful as an interactive figure, after all....

@javiergcas
Copy link
Collaborator

Here is a screenshot of what we are building. It includes 4 plots (from left to right):

  • Kappa vs. Rho Scatter
  • Ranked Kappa
  • Ranked Rho
  • Ranked Variance explained.

All these plots are linked, so that when you click on a component in one plot, the same gets selected in the other three. There is also hovering capabilities, and soon when you click you will also get the corresponing timeseries, fft, and viewer for the component.
Example

@dowdlelt
Copy link
Collaborator Author

dowdlelt commented Nov 12, 2019

its beautiful 😍, the images of my dreams! they even have color coding!

@emdupre emdupre mentioned this pull request Nov 13, 2019
@eurunuela
Copy link
Collaborator

I believe this PR does not make sense anymore, now that we are working on the dynamic reports. Should we close this PR?

@tsalo
Copy link
Member

tsalo commented Nov 21, 2019

Are scree plots included in the current version of the dynamic reports? I agree that this PR should be closed, but we should make sure that the figures still end up being created.

@dowdlelt
Copy link
Collaborator Author

I believe they are - and while I'm excited to have dynamic reports - I also thought the intention was to have these as fallback/optional figures. For that reason I think it would be good for them to be at parity with the dynamic reports.

@jbteves
Copy link
Collaborator

jbteves commented Dec 12, 2019

@emdupre @dowdlelt @tsalo
I believe we should include these as part of the static figures. Unfortunately there is now a merge conflict for the outputs; could you resolve it, @dowdlelt ? Happy to help if needed.

@dowdlelt
Copy link
Collaborator Author

I have my notes, so maybe I can actually pull this off. I'll give it a shot

@jbteves
Copy link
Collaborator

jbteves commented Dec 13, 2019

Awesome, thanks! If you have trouble just remember to reply here instead of Gitter since we're trying to push everything onto issues and PRs where possible.

@dowdlelt
Copy link
Collaborator Author

I've adapted to the minimization of the 'kitchen conversation' scheme better than I had hoped. And with any luck, I've also resolved the merge conflict - fingers crossed for testing.

@jbteves
Copy link
Collaborator

jbteves commented Dec 13, 2019

I think you did it correctly : - )
That wasn't accusatory, BTW, it was just a reminder. To me just as much as you, and to everyone reading this.
It is a lot easier in that it's easy to find replies.

@jbteves
Copy link
Collaborator

jbteves commented Dec 13, 2019

@dowdlelt since figures are now written by default you actually need to add the scree for all three integration tests, not just three-echo. Sorry, should have caught that when I looked over the PR.

jbteves
jbteves previously approved these changes Dec 13, 2019
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.

Excellent, thanks @dowdlelt !

@jbteves jbteves requested review from eurunuela and tsalo December 13, 2019 18:26
tedana/viz.py Outdated Show resolved Hide resolved
tedana/viz.py Outdated Show resolved Hide resolved
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

@dowdlelt dowdlelt merged commit 6345335 into ME-ICA:master Dec 13, 2019
@dowdlelt
Copy link
Collaborator Author

Thanks folks, glad to close this out.

@dowdlelt dowdlelt deleted the new_fig branch December 13, 2019 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues describing possible enhancements to the project hackathon Issues to tackle in the NIH hackathon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants