-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add T2* and S0 figures #1040
Add T2* and S0 figures #1040
Conversation
The brain images are massive in the HTML report for some reason. @eurunuela do you have any idea what might be causing that? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1040 +/- ##
==========================================
+ Coverage 89.55% 89.66% +0.11%
==========================================
Files 26 26
Lines 3466 3503 +37
Branches 634 634
==========================================
+ Hits 3104 3141 +37
Misses 210 210
Partials 152 152 ☔ View full report in Codecov by Sentry. |
Yes. So it looks like you haven't used any CSS to limit the size of the image. It is currently taking 100% of the width of its parent div, which happens to be 100% of the page. I can work on improving the overall layout of the report in the coming weeks. |
Ah, I see you made some edits to the CSS @tsalo, awesome! Let me know if you want me to work on improving this. |
I'm happy to just get something readable working in this PR, but I'd love to improve the report overall in a future one. One thing that would be amazing is if we could have all of the figures be rendered with the same width. Maybe even based on the window size, if that's feasible? |
Yes, that's feasible and shouldn't be difficult to build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for doing this. These are nice additions. In addition to my suggested change below, the colorbars for maps are really narrow and unreadable. I can't figure out how to make them wider.
Also, once the figures are finalized, the "ICA Components Report" section of outputs.rst
in the docs should be updated to mention/explain the new figures.
One other Q is the addition of seaborn. It's a widely used library, but it's one more dependency. It makes the histograms slightly nicer, but I don't know if it's necessary. If you eventually plan to use seaborn to make everything look a bit nicer, that's one thing, but, for this PR, I don't think it's essential. (Not going to protest if you really want to keep the seaborn visualizations) |
That's a good point. I've dropped seaborn. |
I've opened an issue about the colorbar (nilearn/nilearn#4283), so I think we can address that particular problem in a future PR. |
This looks good. The one last thing to do before merging is to add some images and explanatory text to the "ICA Components Report" section of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one annoying suggestion. We have a parameter to select the colormap and we default to a red/blue map for the ICA components with positive and negative values. The T2* and S0 maps are all positive and I find are much more interpretable in grayscale. I'd suggest hard-coding them as gray scale rather than using the inputted colormap. If you disagree, I won't argue.
Also, the other text descriptions give a hint at what people should be looking for. You could add something like "The T2* map should look similar to T2 maps and be brightest in the ventricles. The S0 map should roughly follow the signal-to-noise ratio and will be brightest near the surface near receiver coils."
The T2* histogram should have more values or a second bump to the left of the peak, but I'm not sure what to highlight to users as a warning sign of a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor suggestion for the documentation text and my other suggestion to make the T2* and S0 maps always grayscale. I'm flexible on both, if you have other preferences.
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
@handwerkerd what do you think of the updated version? If you like it, I'll update the image in the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for prolonging this. In addition to suggested changes below, t2s_and_s0_plots.png
should use a grayscale image. Also, I'm fine with that image just including the t2s example, but, given the file name, did you intend for it to also include s0 examples?
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Co-authored-by: Dan Handwerker <7406227+handwerkerd@users.noreply.github.com>
Thanks for humoring my requests. I think the maps look good now so just update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry again for creating so much back-and-forth on this one.
No worries. I'm glad it looks good to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this - the nice visual is a useful addition and a good sanity check. Nice work ya'll - I had completely missed that this was being put together.
Closes #689.
Changes proposed in this pull request:
nilearn.plotting.plot_stat_map
.seaborn.histplot
.