Ipysurfer #31
Ipysurfer #31
Conversation
|
I have started to work on API changes. I have noticed that in PySurfer there is a different dictionary that represents mesh orientation and for me it is not obvious how to go from that representation to MNE-like with azimuth and elevation dictionary. Which one should I stick with? How to go from PySurfer angles toward azimuth, elevation representation? Another question is what should I do with MNE functions, e. g. _limits_to_control_points, that I used previously, should I create a copy of those in GSOC project? |
The important thing is that each entry gives the same view of the brain in both places, so |
|
@OlehKSS let us know when you'd like some 👀 on this! |
|
Thanks, I am going to publish version for the review soon |
|
Hi, I have moved almost everything from the brain_plot = Brain(subject, hemi, surface, size=size,
subjects_dir=subjects_dir, title=title,
foreground=foreground)
brain_plot.add_data(data, min=ctrl_pts[0], mid=ctrl_pts[1],
hemi=h, max=ctrl_pts[2], center=center,
colormap=lim_cmap, alpha=alpha)
brain_plot.show()You can see it in |
|
No it should be separate: https://github.com/nipy/PySurfer/blob/master/surfer/viz.py#L3436 Sorry it's not in the API ref, I'm working on adding it but it's a pain. |
|
Ok, thanks. What about color bar visualization, should that function be a part of |
|
colorbar control should be in |
|
Hello, I have finished major changes to the API, now it is |
|
@larsoner @choldgraf Regarding the fact that in a week and a half I will need to submit the final version of my work, what do you think are the most important things to do, besides this pull request and ipyvolume bug #156? As far as I understand, to meet GSOC requirements I should improve |
|
I am currently traveling, @choldgraf do you have time to look? |
ipysurfer/_utils/colormap.py
Outdated
|
|
||
|
|
||
| def _calculate_cmap(lim_cmap, alpha, ctrl_pts, scale_pts): | ||
| def _calculate_lut(lim_cmap, alpha, min, mid, max, center=None): |
There was a problem hiding this comment.
folders probably don't need to have a _ before them, you can just call it utils
|
I'll take a look at it today! |
|
Hi, has anyone manage to take a look at this pull request? |
|
I'll look at it today - haven't had much time since I've been busy prepping for my wedding in 2 weeks :-O |
ipysurfer/_utils/colormap.py
Outdated
| Color map control points. | ||
| scale_pts : tuple(float) | ||
| Data scale control points. | ||
| min : float |
There was a problem hiding this comment.
probably best not to user "min" and "max" since these would overwrite core python functions. How about c_min and c_max? or vmin and vmax to be consistent with matplaotlib?
There was a problem hiding this comment.
yes, that would be better
There was a problem hiding this comment.
I will change the names to fmin and fmax, since these are the ones used in MNE.
ipysurfer/_utils/funcs.py
Outdated
| def _fast_cross_3d(x, y): | ||
| """Compute cross product between list of 3D vectors. | ||
|
|
||
| Much faster than np.cross() when the number of cross products |
There was a problem hiding this comment.
Are these copied over from MNE or custom for this repo?
There was a problem hiding this comment.
I have used PySurfer source code for these. As far as I understand, ipysurfer should be a stand-alone package, so I was trying not to depend on PySurfer or create a circular dependency with MNE.
There was a problem hiding this comment.
in general I'm fine with this package depending on pysurfer and using some of its utility functions, unless that would make "mayavi" a dependency of ipysurfer. @larsoner do you know if that's a reason not to depend on pysurfer?
There was a problem hiding this comment.
I was avoiding dependency on PySurfer due to possible mayavi involvement.
| floating point time values to strings, or None for no label). The | ||
| default is ``time=%0.2f ms``. | ||
| smoothing_steps : int | ||
| The amount of smoothing |
There was a problem hiding this comment.
could we make this more descriptive? "amount of smoothing" is a bit vague - what exactly does it correspond to? Alternatively if this is a copy of the docstring from the MNE version, we can leave it
There was a problem hiding this comment.
Yes, it's a copy from MNE function. I will leave like that, ok?
There was a problem hiding this comment.
yep - and if any of my other comments are equally relevant to the MNE docstrings, I don't think they need to be fixed here
ipysurfer/stc.py
Outdated
| smoothing_steps : int | ||
| The amount of smoothing | ||
| transparent : bool | None | ||
| If True, use a linear transparency between fmin and fmid. |
There was a problem hiding this comment.
what do "fmin" and "fmid" correspond to?
There was a problem hiding this comment.
These are two control points of a colormap. I will make this statement more clear.
ipysurfer/stc.py
Outdated
| Alpha value to apply globally to the overlay. Has no effect with mpl | ||
| backend. | ||
| time_viewer : bool | ||
| Display ipybolume time slider. |
ipysurfer/stc.py
Outdated
| figure : ipyvolume.Figure | list | int | None | ||
| If None, a new figure will be created. If multiple views or a | ||
| split view is requested, this must be a list of the appropriate | ||
| length. If int is provided it will be used to identify the Mayavi |
There was a problem hiding this comment.
*ipyvolume figure, not mayavi figure, presumably
ipysurfer/viz/brain.py
Outdated
| from .surface import Surface | ||
|
|
||
|
|
||
| class Brain: |
There was a problem hiding this comment.
classes generally inherit from "object" yeah? e.g. https://github.com/mne-tools/mne-python/blob/601a98f580aafbee3ea7687d91ffda7e83b57af5/mne/decoding/mixin.py#L4
There was a problem hiding this comment.
As far as I see, that depends on Python version. In Python 3 I can write class Brain and it implicitly inherit from object. On the other hand, I will need to write class Brain(object) for Python 2. I was using Python 3 for this repo, maybe it will be better to make explicit inheritance from object, in case we would want this module to work with Python 2. What do you think?
There was a problem hiding this comment.
ah I see what you mean - I think we should try and keep things relatively 2.X compatible for now (though it seems like packages are starting to drop 2.X compatibility, I think MNE will lag behind this by a little bit...besides, I'm pretty sure "explicit is better than implicit" is the second line of the Zen of Python :-) )
ipysurfer/viz/brain.py
Outdated
| Color of the foreground (will be used for colorbars and text). | ||
| None (default) will use black or white depending on the value | ||
| of ``background``. | ||
| figure : list of mayavi.core.scene.Scene | None | int |
ipysurfer/viz/brain.py
Outdated
| Attributes | ||
| ---------- | ||
| geo : dict | ||
| Cortex surface objects. |
There was a problem hiding this comment.
clarify if these are ipyvolume objects or something unique to ipysurfer
ipysurfer/viz/brain.py
Outdated
| u"""Display widget.""" | ||
| ipv.show() | ||
|
|
||
| def update_lut(self, min=None, mid=None, max=None): |
There was a problem hiding this comment.
what does "lut" stand for? if the method updates teh colormap, let's just call it "update_colors" or something
There was a problem hiding this comment.
As I know, "lut" stands for "Look-up table". I have borrowed naming from PySurfer. I will change it to something more obvious.
There was a problem hiding this comment.
if this is just copy-pasted from ipysurfer then I think it's fine...
ipysurfer/viz/surface.py
Outdated
|
|
||
|
|
||
| class Surface: | ||
| """Container for a surface. |
There was a problem hiding this comment.
Add a more verbose description of the class underneath the one-liner at the top of the docstring so people can understand in more detail what it does. (same goes for the "Brain" class)
|
Ah I just realized I was leaving all of those comments as "single comments" instead of as a review, so they've already showed up. In general the code looks good, though I'm not really able to debug guts of the STC stuff since I haven't done that work before. I'm still trying to get the examples to run on my machine but it's taken a while to set this up. It sounds like this requires the "master" branch of ipyvolume, yeah? Once I get them running I can give some more comments on the examples themselves, and maybe the API in general. |
|
finally got it working! the visualizations look really slick :-) nicely done! a quick thought: the "time bar" works relatively well, but the 3D visualization seems to lag behind changes in time by a few seconds (e.g. if I hit a different point on the time bar, then the plot takes a few seconds to update). Is this just because I'm on a not-great computer? Here's an example:
|
|
For me it was also lagging. I suppose it is because I do some calculations on the |
|
I have updated the pull request according to the code review. |
|
I do not have time to look closely right now so I trust @choldgraf's judgment :) I do agree that it's better to avoid dependencies and have this be standalone for now. Longer term we might try to get PySurfer to have multiple backends, which this could be one of. However, right now |
|
Nice! I just pulled changes and took another look. A few more points for you @OlehKSS :-)
Let me know if you have any questions or thoughts - really nice job so far, now that most of the features are in place lets make sure they're polished and documented so people can appreciate them! |
|
Hi, @choldgraf,
|
|
I have updated documentation and usage samples according to the latest code review suggestions. |
There was a problem hiding this comment.
Looks good - some small nitpicks in there (and now we need a rebase to make the merge possible) but I think in general we should merge this and iterate in smaller pieces later on. The examples work for me and this is a complex enough PR that I think it's better we get it through. @larsoner what do you think?
ipysurfer/stc.py
Outdated
| @@ -28,9 +28,11 @@ def plot_source_estimates(stc, subject=None, surface='inflated', hemi='lh', | |||
| hemi : str, 'lh' | 'rh' | 'split' | 'both' | |||
| The hemisphere to display. | |||
| colormap : str | np.ndarray of float, shape(n_colors, 3 | 4) | |||
| Name of colormap to use or a custom look up table. If array, must | |||
| Only str type is supported. | |||
There was a problem hiding this comment.
I'm a bit confused here - so you cannot input an array? If so, then we should just remove the np.ndarray ...etc from the first line of this argument. Or, clarify by saying "only str type is currently supported" to imply that it'll be supported in the future
examples/ipysurfer.ipynb
Outdated
| @@ -18,12 +18,12 @@ | |||
| "cell_type": "markdown", | |||
| "metadata": {}, | |||
| "source": [ | |||
| "Initialize a basic visualization session.\n" | |||
| "Initialize a basic visualization sessiona and plot a brain mesh. For this purpose we need to provide subject information, e.g. `subject_id`, `subjects_dir`; which type of freesurfer surface mesh we would like to plot, e.g. 'pialed' or 'inflated'; and which hemisphere to plot or whether to plot both." | |||
ipysurfer/stc.py
Outdated
| @@ -138,14 +140,14 @@ def plot_source_estimates(stc, subject=None, surface='inflated', hemi='lh', | |||
| subject = _check_subject(stc.subject, subject, True) | |||
|
|
|||
| if not isinstance(colormap, str): | |||
| raise NotImplementedError('Support for "colomap" of a type other' + | |||
| ' than str is not yet implemented.') | |||
| raise ValueError('Support for "colomap" of a type other' + | |||
|
@OlehKSS regarding the final report, I'd recommend a blog post that covers the technical things that you had to do in order to make this happen. What were the core pieces of tech that made this package possible? What were some challenges that you had to overcome, and the technical decisions you made to overcome them? I think it'd be interesting and useful for you to tell a short narrative about your experience working on this problem! In addition we can link to interactive examples via Binder and make sure that the readme looks good and everything is documented. |
|
@choldgraf I have changed binder link in |
API changes to be consistent with PySurfer.