Skip to content

Align _AbstractRenderer.tube with the PyVista implementation - #14125

Merged
larsoner merged 3 commits into
mne-tools:mainfrom
karpovantonme:fix/abstract-tube-signature
Aug 3, 2026
Merged

Align _AbstractRenderer.tube with the PyVista implementation#14125
larsoner merged 3 commits into
mne-tools:mainfrom
karpovantonme:fix/abstract-tube-signature

Conversation

@karpovantonme

Copy link
Copy Markdown
Contributor

Reference issue (if any)

None.

What does this implement/fix?

_AbstractRenderer.tube declares

def tube(self, origin, destination, radius=0.001, color="white", scalars=None,
         vmin=None, vmax=None, colormap="RdBu", normalized_colormap=False,
         reverse_lut=False):

but _PyVistaRenderer.tube, the only implementation, ends with opacity=None and uses it (_add_mesh(..., opacity=opacity)), and _plot_sensors_3d calls it that way when drawing the fNIRS source-detector pairs:

actor, _ = renderer.tube(
    origin=sources[ch_name][np.newaxis] * unit_scalar,
    destination=detectors[ch_name][np.newaxis] * unit_scalar,
    radius=0.001 * unit_scalar,
    opacity=sensor_alpha["fnirs_pairs"],
)

So the parameter that the live caller relies on is missing from the contract. This adds opacity=None to the abstract signature, in the same position as the implementation.

Two things in the same docstring were also off:

  • backface_culling is documented but is not a parameter of either signature. It is a real parameter of mesh and surface, just not of tube, so the block looks copied. Removed.
  • normalized_colormap is in both signatures and was not documented. Added, reusing the wording already used for it in contour in the same file.

The abstract method body is pass, so there is no runtime change. No changelog entry, since nothing user-facing changes — happy to add one if you would rather have it recorded.

While reading around this I noticed whiten_evoked documents diag : bool (default False) while the signature has diag=NoneNone was introduced deliberately in ce2486c (#4601), but the body only does if diag: so both values behave the same and the docstring was never updated. That is a separate one-liner and I did not want to mix it in; say the word and I will send it.

AI assistance, per the policy: the mismatch was surfaced by a sweep comparing numpydoc Parameters blocks against actual signatures, run with Claude Opus in Claude Code, which also wrote the patch; it checked the implementations and callers of tube and the history of the lines. I read the diff and the call site before submitting.

This is my second PR here today (the other is #14124) and they are unrelated — apologies if that is more than you would like at once; I am happy to close either and resubmit later.

The abstract signature has no `opacity`, but `_PyVistaRenderer.tube`
does and forwards it to `_add_mesh`, and `_plot_sensors_3d` passes it
when drawing fNIRS source-detector pairs. So the one caller that uses
the parameter is calling something the contract does not declare.

The docstring meanwhile documented `opacity` — which was not in the
signature — and `backface_culling`, which exists in neither, while
`normalized_colormap`, which is in both signatures, was undocumented.

Add `opacity=None` in the same position as the implementation,
document `normalized_colormap`, and drop the `backface_culling` block.
The method body is `pass`, so nothing changes at runtime.
@karpovantonme
karpovantonme requested a review from drammock as a code owner August 3, 2026 13:08
@welcome

welcome Bot commented Aug 3, 2026

Copy link
Copy Markdown

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

@larsoner

larsoner commented Aug 3, 2026

Copy link
Copy Markdown
Member

Seems like a reasonable fix... even if the Renderer interface isn't exactly meant for public use.

Could you add a little docs/changes/dev/14125.bugfix.rst for this? And if you can log in to CircleCI with your GitHub account, it should make the next push run properly

@karpovantonme

Copy link
Copy Markdown
Contributor Author

Changelog entry added, along with the names.inc line.

CircleCI is sorted now, I am logged in. The last run predates that, so it still shows the pipeline never starting. Could you kick it off, or should I push again to trigger it?

@larsoner
larsoner enabled auto-merge (squash) August 3, 2026 23:01
@larsoner

larsoner commented Aug 3, 2026

Copy link
Copy Markdown
Member

I started it. Marking for merge-when-green. Thanks in advance @karpovantonme !

@larsoner
larsoner merged commit 34509ed into mne-tools:main Aug 3, 2026
29 checks passed
natinew77-creator added a commit to natinew77-creator/mne-python that referenced this pull request Aug 4, 2026
The renderer API gained opacity=None in mne-tools#14125, meaning "use the default",
which float() cannot take. Every drawing method here goes through _add, so
translate it there once.
larsoner added a commit to larsoner/mne-python that referenced this pull request Aug 4, 2026
* upstream/main: (35 commits)
  Fix bug with coreg scaling (mne-tools#14132)
  MAINT: Update code credit (mne-tools#14131)
  Fix bugs with dark mode panels (mne-tools#14109)
  Improve code credit workflow (mne-tools#14120)
  Align _AbstractRenderer.tube with the PyVista implementation (mne-tools#14125)
  Assign instead of compare when marking explicit zeros in spatio_temporal_dist_adjacency (mne-tools#14124)
  MAINT: Update pre-commit hook versions (mne-tools#14122)
  MAINT: Update dependency specifiers (mne-tools#14121)
  fix: correct typo in comment (mne-tools#14117)
  [dependabot]: Bump the actions group with 2 updates (mne-tools#14123)
  Fix` read_raw_eyelink()` failure when recording blocks are empty or starting with empty values (mne-tools#13571)
  ENH: Add `event_key` parameter to `read_raw_egi` for MFF event metadata (mne-tools#14086)
  fix for scipy sparse deprecations (mne-tools#14118)
  Fix notch spectrum fit (mne-tools#14116)
  Speed up notch filter spectrum fit (mne-tools#14114)
  fix MNE-RT links and update roadmap (mne-tools#14096)
  Clarify `docdict["filter_length_notch"]` (mne-tools#14113)
  Refactor test_plot_alignment_basic() (mne-tools#13472)
  BUG: Cleanup cHPI filtering using smooth interpolation (mne-tools#14112)
  Use redirector app (mne-tools#14111)
  ...

# Conflicts:
#	mne/tests/test_filter.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants