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
[MRG] Glass brain visualisation #245
Conversation
Exciting! Cc @chrisfilo |
indeed ! really nice ! |
Indeed very exciting! Where is the outline coming from? |
The outline comes from an SVG with a little bit of work to extract the coordinates of the nodes from the SVG and draw the Bézier curves in matplotlib. The SVG was created by Gaël by vectorising and simplifying a bitmap picture. |
This is very nice indeed. Little remark: on the second and third images, something is visible outside of the brain, maybe we should crop that automatically? Have you shown these images to neuroscientists? They usually have very specific needs and, from my experience, they ask for a high threshold so that they can see the anatomical features in the background. Are the guidelines overlayed on the brain sufficient for them? |
I was wondering about where these data points outside the brain were coming from and I was told it may well be the eyes. Those regions can be seen in the plotting examples too for example this one: If there is a way to remove these regions automatically, it's probably worth doing. Something interesting to note, is that the values are pretty high (in this case very negative) and setting a high threshold won't make them go away. Definitely a good idea to get some feedback from neuroscientists at one point in the future. |
I am against removing them: we are doing data viz, we shouldn't be |
That's a valid point indeed but then maybe it's still worth removing these regions in an ad-hoc way in our examples. |
+1 for keeping them. Maybe the subject closed the eyes while pressing the button. |
data[np.isnan(data)] = 0 | ||
|
||
bg_img, black_bg, vmin, vmax = img_plotting._load_anat() | ||
slicer = img_plotting.plot_glass_brain(bg_img, vmin=3000, vmax=2*310983, |
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.
What do these values come from ? It's a bit disturbing for users who don't understand them.
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 tweaked these values by hand to have a nicer color range. To be honest, the only use of this plot is to align the brain schematics with the anatomy and eventually hand-tweak the SVG to make them fit a bit better. I don't think it will stay as an example plot.
@margulies how does this outline looks from a neuroanatomical point of view? |
@@ -572,4 +573,77 @@ def plot_stat_map(stat_map_img, bg_img=MNI152TEMPLATE, cut_coords=None, | |||
**kwargs) | |||
return slicer | |||
|
|||
def plot_glass_brain(stat_map_img, #bg_img=MNI152TEMPLATE, cut_coords=None, | |||
output_file=None, #display_mode='ortho',TODO support display_mode |
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.
pending TODO
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.
There are definitely a few of those TODOs sprinkled in the code. I'll add a note to myself to make sure I tackled all of them before merging.
@@ -832,11 +890,73 @@ class YZSlicer(OrthoSlicer): | |||
z=ZSlicer) | |||
|
|||
|
|||
class OrthoProjector(OrthoSlicer): |
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 am still inheriting from the cut-based slicer and disabling a few methods which is rather hacky. There must be a better way to structure this code, suggestions welcome!
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 am still inheriting from the cut-based slicer and disabling a few methods
which is rather hacky. There must be a better way to structure this code,
suggestions welcome!
I would say: add a base class, and inherit from it in the OrthoSlicer,
where you add methods, and in your own projector, where you add different
methods.
You should paste here the results of the latest iteration of the code. |
def _codes_segment(self, pts): | ||
return [Path.MOVETO, Path.LINETO] | ||
|
||
@staticmethod |
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.
Is there a reason to have a staticmethod, rather than a function, here? (ie something not linked to the class).
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.
The main reason is that this is a helper function which is only used within the class. I could change it to a module-level function but you would then lose the hint.
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.
The main reason is that this is a helper function which is only used
within the class. I could change it to a module-level function but you
would then lose the hint.
But it's not a method. An object is more than a namespace of methods. It
has an internal state and methods act on its internal state. That's why
reading code that is object-oriented is harder to read: you never know
what has changed and what has not.
Thus, I would really push for using functions rather than staticmethods.
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.
Yeah I think we quickly chatted about that already before. I just see this low-level helper functions as not important enough to warrant polluting the module namespace. During a first read of the code, I feel it is easier to ignore these unnecessary details if they are nested within the class they are related to.
d772b01
to
080b920
Compare
@GaelVaroquaux I think I tackled the comments you had the other day |
@@ -6,8 +6,10 @@ | |||
""" | |||
|
|||
import operator | |||
import os |
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.
Unused import
You should use pyflakes in your editor, it is great to detect these things. See http://scipy-lectures.github.io/advanced/debugging/index.html#a-type-as-go-spell-checker-like-integration
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.
Actually something like this configured already. I just didn't spot this one.
We should rename 'slicers.py'. But we need a better name. Any suggestions? |
|
||
def draw_cut(self, cut, data_bounds, bounding_box, | ||
type='imshow', **kwargs): | ||
def draw(self, cut, data_bounds, bounding_box, |
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.
Good idea to rename the method!
I think that I would prefer it to be named something like 'draw_volume'. Also, we should probably make it private: it's for the internals of the object, not for end-users to call.
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.
Please disregard the comment on making it private: I hadn't seen that it was in our Axes object. End users are not meant to use this object anyhow, so let's not bother.
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.
The thing is that draw
actually draws some 2d data. I could rename it draw_2d
to make it more explicit.
As a side comment I also plan to go over the file and remove the cut*
occurrences where easily doable.
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.
The thing is that draw actually draws some 2d data. I could rename it draw_2d
to make it more explicit.
Yes, that's a good suggestion.
As a side comment I also plan to go over the file and remove the cut*
occurrences where easily doable.
Good idea. Thanks!
I was thinking about displays.py |
* Use it in the glass brain plot by default * Minort tweaks in plot_glass_brain.py to use the default color map
Otherwise the colormap goes gray towards the low alpha values
depending on black_bg. Also black_bg default to False because 'auto' makes sense only when plotting an anat. Updated plot_glass_brain.py accordingly.
- Only three kind of stroke-widths: 1.2, 2.5, 3 - Did some ungrouping in brain_schematics_top.svg - Regenerated JSONs
in brain_schematics_top.svg. Regenerated JSON
and regenerated JSONs
Also: * create_display_fun renamed to display_factory * removed unused import * PEP8 fix
Also other minor tweaks
in plot_glass_brain in order to be aligned with brain schematics
This is the value that was used in all the latest screen snapshots from the PR
This script was mostly useful to produce the snapshots in the PR
I saw that, but I prefer it with the alpha. |
OK merging this long standing one ! |
[MRG] Glass brain visualisation
Fucking Ay! Can you rebuild the online docs, please; |
added output_image argument to plot_design_matrix
This pull request is WIP for now and was only opened to get some feedback, both visualisation-wise and code-wise.
Here is how it looks like atm:
The code to generate the plots is there.