ENH: Add kwargs in Brain.add_* and pass it to mayavi modules #276
Conversation
…t we could directly manipulate mayavi display settings, like point resolution and overlay opacity.
DingkunLiu
changed the title
DingkunLiu
changed the title
surfer/viz.py
Outdated
| @@ -932,6 +932,7 @@ def add_overlay(self, source, min=2, max="robust_max", sign="abs", | |||
| If None, it is assumed to belong to the hemipshere being | |||
| shown. If two hemispheres are being shown, an error will | |||
| be thrown. | |||
| kwargs: other mayavi surface arguments. | |||
There was a problem hiding this comment.
This should follow the NumPy docstring format, like:
kwargs : dict
Other Mayavi surface arguments.
There was a problem hiding this comment.
But it is not necessarily to be a dict here. How about changing it to "All additional keyword arguments are passed to the
mlab.pipeline.surface call." and specifying the name kwargs to mlab_kws?
Or I could just remove the ** symbols to make it a real dict?
There was a problem hiding this comment.
Let's do what nibabel does (except we don't need to escape the star characters)
https://github.com/nipy/nibabel/pull/827/files#diff-a1d0ba69368622536897ef9fe4aee05eR766
There was a problem hiding this comment.
This link is better (sorry on mobile)
There was a problem hiding this comment.
There are a lot of detailed arguments for each mayavi function, so I think it is hard to list all the possible arguments. Could we just list some useful examples?
There was a problem hiding this comment.
And one more thing, I think we should use the same name for the same argument in different functions. For example, the alpha in add_data() in mayavi is called opacity, so I think we should also specify this name in other functions and avoid it to appear in mlab_kws.
There was a problem hiding this comment.
I don't think we need to list all mayavi keywords, but it would be great if you could enable intersphinx so that the docs produce a link to the relevant mayavi api doc.
Nor do I think that we should have mlab_kws; the convention is to collect extra keyword arguments with **kwargs, and "mlab" isn't particularly informative about where the kwargs are actually going.
I'd also like to minimize changes unrelated to this specific enhancement. There is some inconsistency between pysurfer parameter names and mayavi ones, but aiming to fully streamline that would be a rabbit hole that is best to avoid.
There was a problem hiding this comment.
Agreed, I would just do:
**kwargs : additional keyword arguments
These are passed to the underlying :func:`mayavi.mlab.surface` call.
To figure out the links you can dump the intersphinx inventory with something like:
$ python -msphinx.ext.intersphinx http://docs.enthought.com/mayavi/mayavi/objects.inv > mayavi.txt
Then browse for relevant entries in mayavi.txt. Unfortunately they might not have entries for each function you actually want to use.
There was a problem hiding this comment.
I have updated the documents, but unfortunately, those for the functions in mlab.pipeline are missing.
…more clear." This reverts commit c7a5912.
Codecov Report
@@ Coverage Diff @@
## master #276 +/- ##
==========================================
+ Coverage 73.96% 74.31% +0.35%
==========================================
Files 7 7
Lines 2535 2570 +35
Branches 511 514 +3
==========================================
+ Hits 1875 1910 +35
Misses 482 482
Partials 178 178 |
This reverts commit 2c86a04.
surfer/viz.py
Outdated
| @@ -932,6 +932,10 @@ def add_overlay(self, source, min=2, max="robust_max", sign="abs", | |||
| If None, it is assumed to belong to the hemipshere being | |||
| shown. If two hemispheres are being shown, an error will | |||
| be thrown. | |||
| **kwargs: additional keyword arguments | |||
| These are passed to the underlying | |||
| :func:`mayavi.mlab.pipeline.surface` call. | |||
There was a problem hiding this comment.
This extra indentation makes for weird spacing, see CircleCI doc artifact:
| :func:`mayavi.mlab.pipeline.surface` call. | |
| :func:`mayavi.mlab.pipeline.surface` call. |
https://292-1551431-gh.circle-artifacts.com/0/html/generated/surfer.Brain.html#surfer.Brain.add_foci
Also many links do not work:
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_annotation:37: WARNING: py:func reference target not found: mayavi.mlab.pipeline.surface
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_contour_overlay:42: WARNING: py:func reference target not found: mayavi.mlab.pipeline.contour_surface
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_data:105: WARNING: py:func reference target not found: mayavi.mlab.pipeline.surface
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_foci:41: WARNING: py:func reference target not found: mayavi.mlab.point3d
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_label:43: WARNING: py:func reference target not found: mayavi.mlab.pipeline.surface
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_morphometry:33: WARNING: py:func reference target not found: mayavi.mlab.pipeline.surface
/home/circleci/project/surfer/viz.py:docstring of surfer.Brain.add_overlay:29: WARNING: py:func reference target not found: mayavi.mlab.pipeline.surface
These should just be put in double-backticks to render as code. Or do the intersphinx dump and find a link that exists in the objects.inv.
There was a problem hiding this comment.
At least one is just a typo, point3d should be points3d so it directs here
https://docs.enthought.com/mayavi/mayavi/auto/mlab_helper_functions.html#points3d
Fixing these errors is required to get CircleCI happy
There was a problem hiding this comment.
Sorry for the late reply. I was a little overloaded these days. I just pushed a new commit. And, gladly, the errors seem to be solved.
This reverts commit de31e5f.
Force line breaking to avoid render error.
|
Thanks @DingkunLiu |
Add kwargs in Brain.add_* and pass it to mayavi modules, so that we could directly manipulate mayavi display settings, like point resolution and overlay opacity.