mne-tools / mne-gsoc2018-3d

Sandbox for GSoC 2018 3D Viz
BSD 3-Clause "New" or "Revised" License
9 stars 4 forks source link

Ipysurfer #31

Closed OlehKSS closed 6 years ago

OlehKSS commented 6 years ago

API changes to be consistent with PySurfer.

OlehKSS commented 6 years ago

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?

larsoner commented 6 years ago

Which one should I stick with? How to go from PySurfer angles toward azimuth, elevation representation?

The important thing is that each entry gives the same view of the brain in both places, so lat should look from the side (from the left or right depending on hemi). To get what this angle dict (which is for Mayavi) means in IPyvolume I don't know, but you can figure it out using trial and error if you really need to (i.e., for each view, do it in PySurfer, then figure out what you need to do in IPyvolume to make the same view).

choldgraf commented 6 years ago

@OlehKSS let us know when you'd like some 👀 on this!

OlehKSS commented 6 years ago

Thanks, I am going to publish version for the review soon

OlehKSS commented 6 years ago

Hi, I have moved almost everything from the mne_g3d to ipysurfer. For creating a plot and adding activation data ones should use snippet:

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 stc.plot_source_estimates sample function. How should time viewer should be added? Should it be a part of Brain class?

larsoner commented 6 years ago

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.

OlehKSS commented 6 years ago

Ok, thanks. What about color bar visualization, should that function be a part of Brain class or should it go to the TimeViewer as well?

larsoner commented 6 years ago

colorbar control should be in Brain

OlehKSS commented 6 years ago

Hello, I have finished major changes to the API, now it is ipysurfer instead of mne_g3d. The plotting capabilities are the same as they were. You can see usage sample in this file. Could take a look at the proposed changes?

OlehKSS commented 6 years ago

@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 read.md file, so it will contain project description, use examples, maybe some screenshots and share the link to the repository. What else should I do to prepare my project for the final evaluation/submission?

larsoner commented 6 years ago

I am currently traveling, @choldgraf do you have time to look?

choldgraf commented 6 years ago

I'll take a look at it today!

OlehKSS commented 6 years ago

Hi, has anyone manage to take a look at this pull request?

choldgraf commented 6 years ago

I'll look at it today - haven't had much time since I've been busy prepping for my wedding in 2 weeks :-O

choldgraf commented 6 years ago

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.

choldgraf commented 6 years ago

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:

2018-08-06_23-48-39

OlehKSS commented 6 years ago

For me it was also lagging. I suppose it is because I do some calculations on the Python side, not on JavaScript and communication between the two happens through the server. As a result the frame rate is quite low.

OlehKSS commented 6 years ago

I have updated the pull request according to the code review.

larsoner commented 6 years ago

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 PySurfer requires Mayavi so it's worth copy-pasting code a bit to avoid needing to satisfy that dependency.

choldgraf commented 6 years ago

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!

OlehKSS commented 6 years ago

Hi, @choldgraf,

OlehKSS commented 6 years ago

I have updated documentation and usage samples according to the latest code review suggestions.

choldgraf commented 6 years ago

@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.

OlehKSS commented 6 years ago

@choldgraf I have changed binder link in README.MD to lead to the ipyserfer.ipynb file instead of brain-mesh.ipynb, since the latter one is outdated and I have deleted it. Do I need to add the ipyserfer.ipynb file to binder or it will figure out which file to show automatically?