NF: added the option to specify alpha transparency for Brain binarized curvature rendering

[ctw]

I have added the option to specify alpha transparency for Brain binarized curvature rendering. This can be useful when plotting foci in internal structures that are otherwise obscured by the brain surface. I had considered adding an "alpha" keyword to the Brain interface, but opted instead to allow for alpha transparency to be specified in the cortex keyword. I've implemented the chance in a way that should ensure backward compatibility with code specifying the cortex keyword as a 4-tuple or string (in these cases alpha defaults to 1.0). Using a new alpha transparency keyword for the Brain constructor might make this feature more accessible and I'm happy to change the implementation accordingly if that's preferred.

[mwaskom]

Using a new alpha transparency keyword for the Brain constructor might make this feature more accessible and I'm happy to change the implementation accordingly if that's preferred.

I think this would be my preference, but others can comment. It's also possible that opacity would be a more obvious parameter name.

[mwaskom]

Also one weird thing about implementing it through the curvature overlay is that I would think in most cases when you want a semi-transparent cortical mesh you don't want to be showing curvature (it will only complicate things).

[ctw]

I agree that it's a bit annoying that one can only specify alpha when curv is True in the current implementation (although by specifying sufficiently large absolute values for vmin and vmax one can effectively eliminate the contrast). Will change to use a keyword argument unless others object. It seems like "alpha" is the standard keyword argument for transparency (e.g., in add_data, add_annotation, add_label, and add_foci), that's why I chose it over opacity.

[larsoner]

+1 for keyword

[ctw]

On further reflection, I'd like to refactor the interface to Brain a bit more as follows:

I would like to deprecate the curv keyword in favor of allowing the cortex keyword to accept colors (in addition to the currently allowed inputs). If a color is specified, a solid brain (without the curvature) is drawn in that color. This would add the new functionality of allowing the user to specify the color a solid brain is drawn in and eliminate one keyword.

To summarize I propose the following changes:

• add alpha keyword to specify transparency of the brain surface.
• keep default value and behavior for currently accepted inputs to cortex as is, but also allow specification of a color (rather than color map) to produce a solid brain surface (without curvature) in that color. If cortex is set to None the same output is produced as if curv is set to False in the current interface.
• keep curv for backward compatibility, but set the default to None and throw a warning that it is deprecated and will be removed in a future version if it is changed from the default value.

Any thoughts?

[agramfort]

no objection

[ctw]

I've made the proposed changes. Please let me know if you have questions and/or suggestions for improvements.

[mwaskom]

Please make sure that you update the pysurfer script to reflect this changes and also that you add a note to the CHANGES file.

You will also want to look at the example scripts and make sure that none of them are using curv. Then you should probably add an example script to show a semi-transparent brain (perhaps demonstrating how to add subcortical foci). It would also be good to update at least one other example script to cover the new functionality for specifying the curvature values.

[ctw]

I've made the suggested changes. None of the existing example scripts were affected by the changes, but I've added a new one where alpha transparency is specified and a color is passed to cortex instead of a colormap.

[coveralls]

Coverage decreased (-0.4%) to 76.602% when pulling 5af2dad7c8b080b3554c327d042133c442ef839e on ctw:addalpha into d6350fd9192dd8d4cccfef92706dd5d7cb217010 on nipy:master.

[mwaskom]

Thanks! Would you mind showing the output of the example script here in the thread so I don't need to pull down your changes and build the docs?

[mwaskom]

I guess one thing to think about (though it goes against my earlier concerns that the cortex= parameter is too complicated: instead of (or complementing) the (colormap, min, max, reverse) tuple, a much better API might just be a pair of colors giving the color of sulci and gyri. It would be easy to take that pair and generate a colormap with our existing code. Does anyone have thoughts about this option?

[coveralls]

Coverage decreased (-0.5%) to 76.564% when pulling 48bee78e3e9ee7b5da9ae48bf86f9031a1ca414a on ctw:addalpha into d6350fd9192dd8d4cccfef92706dd5d7cb217010 on nipy:master.

[ctw]

I'm having trouble building the docs, at least partly, it seems, because I don't have freesurfer installed. Is there another way?

I like the option of being able to pass two colors to cortex, but the subsequent call to mlab.pipeline.surface expects the colormap to be specified by a string, so getting this to work would seem to require some refactoring.

[mwaskom]

I like the option of being able to pass two colors to cortex, but the subsequent call to mlab.pipeline.surface expects the colormap to be specified by a string, so getting this to work would seem to require some refactoring.

I think you can take a look in Brain.add_data to see how custom colormaps are set there.

[larsoner]

Otherwise the arguments look reasonable to me

[coveralls]

Coverage decreased (-0.5%) to 76.564% when pulling 498f27e471cfeff390d2fdc860fcbdc80f7d0bd8 on ctw:addalpha into d6350fd9192dd8d4cccfef92706dd5d7cb217010 on nipy:master.

[ctw]

I've added the option to pass a list of colors as argument for cortex as suggested by @mwaskom.

[larsoner]

Style:

https://travis-ci.org/nipy/PySurfer/jobs/144918615#L826

[coveralls]

Coverage decreased (-0.4%) to 76.601% when pulling de69f0a9bbabf5bb42a72d12b968e2ca0f32095c on ctw:addalpha into d6350fd9192dd8d4cccfef92706dd5d7cb217010 on nipy:master.

[ctw]

Thanks @Eric89GXL -- fixed now!

[larsoner]

Looks like there are a number of un-covered relevant lines, can you write some simple tests that at least touch them? Can't really test the output but it can at least make sure some inputs get parsed.

[larsoner]

Otherwise LGTM, +1 for merge. Example looks cool.

[larsoner]

Ahh good I see "small fix" in the commit message to justify my nitpick :)

[larsoner]

PEP8 line length

https://travis-ci.org/nipy/PySurfer/jobs/145638217#L826

[coveralls]

Coverage increased (+0.07%) to 77.094% when pulling f44f5d7eb564c4b409bc72e0a9659d95b851ffea on ctw:addalpha into d6350fd9192dd8d4cccfef92706dd5d7cb217010 on nipy:master.

[larsoner]

@mwaskom feel free to merge if you're happy, too

[ctw]

@mwaskom any chance this can be merged?

[mwaskom]

Hi, sorry. I started my postdoc last month and also just adopted a dog so I'm dropping things left and right. But if this looks good to @Eric89GXL it looks good to me.