nipy / PySurfer

Cortical neuroimaging visualization in Python
https://pysurfer.github.io/
BSD 3-Clause "New" or "Revised" License
243 stars 97 forks source link

Show both hemispheres #42

Closed christianbrodbeck closed 11 years ago

christianbrodbeck commented 11 years ago

It has come up repeatedly that it would be desirable to be able to show/manage both hemispheres with a single class. (https://github.com/mne-tools/mne-python/pull/500#issuecomment-14312084, https://github.com/mne-tools/mne-python/pull/418#issuecomment-12952970).

1) I have been experimenting with a "parent" class that contains .lh and .rh, both Brain instances, and 'wraps' commands such as saving images, and has a show method that accepts values like 'lh medial' which would hide the right hemisphere. That has the advantage of leaving the current Brain class intact, but probably has to duplicate a lot of the methods.

What would be other options?

christianbrodbeck commented 11 years ago

2) Based on @mluessi's comment , an option would be to rewrite the Brain class into a "Hemisphere" class that is itself agnostic to the window it is displayed in, "so you can have multiple hemis in the same 3D space and show different things on them etc.". That would be some major changes, but might end up being a cleaner/better implementation.

dengemann commented 11 years ago

@christianmbrodbeck Your first proposal / experimental class sound like what I had outlined last on mne-tools/mne-python#500. It's appealing as it leaves the Brain class intact. I don't see what the second proposal would add given a minimum change scenario leaving the users with a list of Brain instances plotted in one scene. You would then simply adjust each of the Brains separately and this would satisfy the scenario outlined by @mluessi. Would this be more than just renaming Brain to Hemisphere? Btw. The 'minimum change' scenario just outlined would be option 3.

dengemann commented 11 years ago

2) sounds like making your parent Brain with wrapping the new Hemisphere classes which would basically contain stuff from the current Brain class.

christianbrodbeck commented 11 years ago

Adding numbers to keep track...

I guess a difference is a certain degree of separation between model and view, i.e. whether the current Brain class should retain figure management functionality such as saving movies, modifying lighting, ..., or whether that should all be moved to a scene manager class.

agramfort commented 11 years ago

since the only use case I see is showing the two hemispheres why not allowing hemi to be 'both' and let the Brain class do the necessary magic to make this work. We would not need any other new class.

agramfort commented 11 years ago

the time viewer with multiple brain instances could be used to view multiple subjects data at the same time.

christianbrodbeck commented 11 years ago

since the only use case I see is showing the two hemispheres why not allowing hemi to be 'both' and let the Brain class do the necessary magic to make this work. We would not need any other new class.

@agramfort I like that from a user interface perspective. For the implementation,

Either way, backwards compatibility would be broken for manual access to the overlays dictionary. The alternative of making Overlay also being able to manage both hemispheres seems like more redundancy again... (since one object would have manage several plot objects)

agramfort commented 11 years ago

@agramfort I like that from a user interface perspective. For the implementation,

Would we have one Surface object per hemisphere? I.e., Brain._geom_lh and Brain._geom_rh? How should attributes like Brain.overlays and Brain.foci be organized? On the one hand they could just all be duplicated (Brain.overlays_lh, ...) but that would make managing them unnecessarily complicated, so an alternative would be to create an intermediate object Hemisphere that manages the geom and the corresponding overlays etc.? or ... ?

what I would try is to have only one surface which is the concatenation of both. so nothing changes in terms of API. The overlay array would be twice bigger etc.

option 2 is to make attributes lists of len 2 (one entry per hemisphere)

dengemann commented 11 years ago

On 05.03.2013, at 09:30, Alexandre Gramfort notifications@github.com wrote:

@agramfort I like that from a user interface perspective. For the implementation,

Would we have one Surface object per hemisphere? I.e., Brain._geom_lh and Brain._geom_rh? How should attributes like Brain.overlays and Brain.foci be organized? On the one hand they could just all be duplicated (Brain.overlays_lh, ...) but that would make managing them unnecessarily complicated, so an alternative would be to create an intermediate object Hemisphere that manages the geom and the corresponding overlays etc.? or ... ?

what I would try is to have only one surface which is the concatenation of both. so nothing changes in terms of API. The overlay array would be twice bigger etc.

option 2 is to make attributes lists of len 2 (one entry per hemisphere)

this is what i had already started with and somehow i did not like what it did to the readability of the code...

— Reply to this email directly or view it on GitHub.

mluessi commented 11 years ago

I like the idea of using only one surface as suggested by @agramfort. It seems like you would only need to combine the lh/rh meshes in the constructor and methods which have a vertices argument (e.g. add_data) would need to be extended to accept a tuple/list with two arrays.

dengemann commented 11 years ago

Sounds like the way to go. This proposal will keep the API cleaner and will require less edits.

On Tue, Mar 5, 2013 at 1:47 PM, Martin Luessi notifications@github.comwrote:

I like the idea of using only one surface as suggested by @agramforthttps://github.com/agramfort. It seems like you would only need to combine the lh/rh meshes in the constructor and methods which have a vertices argument (e.g. add_data) would need to be extended to accept a tuple/list with two arrays.

— Reply to this email directly or view it on GitHubhttps://github.com/nipy/PySurfer/issues/42#issuecomment-14438134 .

christianbrodbeck commented 11 years ago

I would like to be able to hide one hemisphere flexibly after plotting (e.g. to look at the medial surface). This should happen automatically when e.g. doing .show_view('lh medial'). I think that requires there to be a separate triangular_mesh for each hemisphere, right?

mluessi commented 11 years ago

@christianmbrodbeck this is a good point. It would indeed be difficult to do this using a single mesh. I was first thinking one could maybe use a cut plane to only show one hemi. I think it may work for inflated surfaces but for e.g. the "white" surface the vertices on the medial wall are very close so it doesn't seem a cut plane would do the job.

larsoner commented 11 years ago

I'm looking into it, and I think it would be fairly simple to show a single mlab window with multiple views of the same brain (say, medial and lateral from the left hemisphere simultaneously). What I'm thinking is it would be useful to have a plot like this:

LLat | RLat
LMed | RMed
LVen | RVen

Or whatever configuration a user wanted. Thus the left hemisphere and right hemisphere would be shown in separate plots, but you could see the entire brain in one go. Perhaps this could be hemi='split', whereas hemi=both would show the hemispheres in proper anatomical proximity (i.e., arranged so that the medial walls are touching). Would other people find this useful?

If so, I can try to make the API such that it's backward compatible. If for arguments like hemi=split and view='lat', you'd get:

LLat | RLat

For hemi='split' and view=['lat', 'med', 'ven'], you'd get the example above. Make sense?

larsoner commented 11 years ago

@dengemann I know this doesn't allow looking at two conditions simultaneously as you wanted, but to my mind it seems cleaner / much simpler to have each subject actually have their own Brain instance -- showing two or more subjects using the same Brain instance would require a major overhaul (probably beyond the scope of PySurfer, and in violation of the name of the object :) ).

However, it might very well be possible to abstract another layer to have multiple Brain views in the same window. That wouldn't be the efficient approach for my proposal (showing each hemi using multiple views) since the data for a single subject could be shared across views of a given hemisphere (thus a single Brain instance should capture all parameters of a given subject), but it's possible it would be easy to add another layer of abstraction, if you want, for multiple subjects.

dengemann commented 11 years ago

On Fri, Mar 15, 2013 at 4:30 PM, Eric89GXL notifications@github.com wrote:

@dengemann https://github.com/dengemann I know this doesn't allow looking at two conditions simultaneously as you wanted, but to my mind it seems cleaner / much simpler to have each subject actually have their own Brain instance -- showing two or more subjects using the same Brain instance would require a major overhaul (probably beyond the scope of PySurfer, and in violation of the name of the object :) ).

Maybe I wasn't explicit enough, I was talking about 2 brains of the same subject with different activations loaded.

However, it might very well be possible to abstract another layer to have multiple Brain views in the same window. That wouldn't be the efficient approach for my proposal (showing each hemi using multiple views) since the data for a single subject could be shared across views of a given hemisphere (thus a single Brain instance should capture all parameters of a given subject), but it's possible it would be easy to add another layer of abstraction, if you want, for multiple subjects.

Besides this, would it harm to just have the figure params in stc.plot so we can, in addition to whatever PySurfer sophisitcation tell which figure to plot on? If the params would make the stc.plot() return 2 brains we would require a list of figs if figure is not None. Makes sense?

Reply to this email directly or view it on GitHubhttps://github.com/nipy/PySurfer/issues/42#issuecomment-14967157 .

larsoner commented 11 years ago

Yeah, that makes sense. Having two different Brain instances for the same subject makes sense for the case where you'd want to plot two different activations simultaneously. It should work well with what I'm scheming.

larsoner commented 11 years ago

Merged in recent PR, closing.