[enh] Dynamic reports refactored following emdupre guidelines #467
Conversation
enh:initial commit of dynamic plots
…wo digits and percent sign for variance.
Codecov Report
@@ Coverage Diff @@
## master #467 +/- ##
==========================================
- Coverage 82.22% 60.5% -21.73%
==========================================
Files 39 47 +8
Lines 2594 3684 +1090
==========================================
+ Hits 2133 2229 +96
- Misses 461 1455 +994
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
As a general comment I wouldn't remove the possibility of making figures, just making them not the default (as it was before). The default can be the report, but while that's a great instrument for single subjects and deeper understanding of the results, it's really not helpful in the case of big datasets (multiple subjects/sessions/runs), cause then the intention is to have a good summary of your results to go through with the least time spent on user interface actions (clicks/navigation/...).
If you think that's more a "post_tedana" thing, we can reintegrate it in a post_tedana workflow, but for the moment I would keep everything in the main workflow, as it shouldn't create conflicts.
| <body> | ||
| $body | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
| </html> | |
| </html> | |
There was a problem hiding this comment.
Same comment, it looks like you're just suggesting adding a blank line ? Not best-practice for HTML.
There was a problem hiding this comment.
Sorry, my bad - you're right, just suggesting padding with a blank line. This should only be done in python scripts though, right? Are we enforcing it or no problem if we don't?
There was a problem hiding this comment.
Thanks for this, @eurunuela and @javiergcas !
I think I'm a little confused -- we don't want both the KappaRho_DynPlot.py file and the dynamic_figures.py file. If KappaRho_DynPlot is intended to supersede dynamic_figures, can we remove the current dynamic_figures file and rename KappaRho_DynPlot as "dynamic_figures" ?
| <body> | ||
| $body | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
Same comment, it looks like you're just suggesting adding a blank line ? Not best-practice for HTML.
| @@ -0,0 +1,578 @@ | |||
| # --- | |||
There was a problem hiding this comment.
@javiergcas I think this file should be removed and its functionality refactored into the dynamic_figures.py file !
There was a problem hiding this comment.
Correct. Sorry I just commented below in the wrong comment for this question
|
|
||
|
|
||
| # %% | ||
| # ---------------------------------------------------------------- |
There was a problem hiding this comment.
This section was me trying to get something to quickly test and show. we should remove everything below L335, here.
There was a problem hiding this comment.
@emdupre: I think I'm a little confused -- we don't want both the KappaRho_DynPlot.py file and the dynamic_figures.py file. If KappaRho_DynPlot is intended to supersede dynamic_figures, can we remove the current dynamic_figures file and rename KappaRho_DynPlot as "dynamic_figures" ? Correct, KappaRho_DynPlot supersedes dynamic_figures
By "figures" I'll assume you mean static figures, here? If so, the brainsprite directories should still provide static figures with visualizations of the components, just not their time series or FFT. |
|
Yes, but spatial maps are not enough to interpret the components. In order to evaluate an IC, the minimal information needed is timeseries and/or FFT and spatial maps. I would even say timeseries are more informative than maps sometimes. I might be very passionate about this point because it's fundamental to my preprocessing, but I don't think I'm the only one that would like to go through many components in a way that is between "having a good idea about what a component represents" and "not loosing too much time in navigating a 3d representation and/or setting up the visualisation". Yet again, if you prefer the figures output to be a post-tedana output (although I don't see the reason for that), I'm sure that @dowdlelt and I can work on making it that way. |
I'd prefer we don't immediately leap to that as a discussion point -- I don't think it's productive to carry the conversation forward. I also don't think these points strongly relate to the actual code under consideration (no points to line numbers, etc) so I'll ask that we revisit this discussion once the code firms up a bit. In general, I'm all for passionate discussions in pull requests but let's keep them productive and focused on the code. |
|
@62442katieb 's PR for linked brainsprites ! : https://github.com/emdupre/tedana/pull/21/files |
|
This issue has been automatically marked as stale because it has not had any activity in 90 days. It will be closed in 600 days if no further activity occurs. Thank you for your contributions to tedana:tada: ! |
|
This was superseded by #457, so I'm going to close it ! Thanks everyone for your contributions, here !! |
Closes # .
Changes proposed in this pull request: