Skip to content

DOC: Remove invalid ndarray option from colormap docstrings#13797

Open
Gnefil wants to merge 3 commits intomne-tools:mainfrom
Gnefil:fix-stc-colormap-doc
Open

DOC: Remove invalid ndarray option from colormap docstrings#13797
Gnefil wants to merge 3 commits intomne-tools:mainfrom
Gnefil:fix-stc-colormap-doc

Conversation

@Gnefil
Copy link
Copy Markdown

@Gnefil Gnefil commented Mar 29, 2026

Reference issue

Fixes #13189 .

What does this implement/fix?

This PR solves the conflict between the documentation and the actual signature of mne.viz.plot_source_estimates, which doesn't accept a numpy array for colormap.

Additional information

A possible future minor enhancement would be to include a numpy array option and transform to colormap using matplotlib as suggested by larsoner in comments. If agreed, that could be my next PR.

Additionally, this is my first time contributing. I looked at the contributing guidelines, played with the code, and viewed how others create issues and PRs. But still, I might miss something, so don't hesitate to point out any mistakes! Finally, this is a work by Lifeng, a 2026 GSoC applicant for project 2.

@Gnefil Gnefil marked this pull request as ready for review March 29, 2026 17:27
Copilot AI review requested due to automatic review settings March 29, 2026 17:27
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates MNE-Python documentation to match the actual accepted types for the colormap parameter (fixing a mismatch where docs claimed np.ndarray was allowed, but runtime validation rejects it), and adds new-contributor attribution for the changelog.

Changes:

  • Update the shared colormap parameter docstring template to remove np.ndarray and document Matplotlib Colormap objects instead.
  • Add a Towncrier dev changelog entry describing the documentation fix.
  • Add the contributor name/link to doc/changes/names.inc.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
mne/utils/docs.py Adjusts the canonical colormap parameter doc snippet to reflect accepted types.
doc/changes/dev/13797.other.rst Adds a changelog entry for the documentation correction.
doc/changes/names.inc Registers the new contributor for changelog attribution.

@CarinaFo
Copy link
Copy Markdown
Contributor

Hi Gnefil,

thank you for your contribution.

If you sign up for a free account on circleci and link it to your github repo, the workflows related to documentation building will run (they are currently failing).

@Gnefil
Copy link
Copy Markdown
Author

Gnefil commented Mar 30, 2026

Hii @CarinaFo, thanks for replying!

If you sign up for a free account on circleci and link it to your github repo, the workflows related to documentation building will run (they are currently failing).

I have successfully signed up for CircleCI, however I am stucked in "No CircleCI organizations found" and "Join via invite". Account integration doesn't show GitHub, so I presume it is linked. This is after Join via GitHub option after signing up. Could you clarify what should be the next steps?

Thanks in advance!

Copy link
Copy Markdown

@JiwaniZakir JiwaniZakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docdict["colormap"] entry in mne/utils/docs.py is a shared docstring used across multiple functions, not just plot_source_estimates — so the changelog entry in 13797.other.rst likely understates the scope of this change. It would be worth auditing all callers of this docdict key (e.g., with a grep for "colormap" in the codebase) to confirm they all accept matplotlib.colors.Colormap and no longer accept raw np.ndarray inputs.

More critically, this PR updates documentation without a corresponding code change to the underlying implementation. If any of the functions that use this docstring still accept and handle np.ndarray arguments internally (as the old docstring described), the new docstring is now inaccurate and will silently mislead users who pass arrays — a behavioral regression in documentation fidelity. Conversely, if ndarray support was already broken or unimplemented, a deprecation notice or explicit error with a clear message would be more user-friendly than silently dropping it from the docs.

Also, 13797.other.rst is missing a newline at end of file, which may cause formatting issues in the rendered changelog.

@Gnefil
Copy link
Copy Markdown
Author

Gnefil commented Mar 31, 2026

Hii Zakir,

Thanks for your critical comments!

so the changelog entry in 13797.other.rst likely understates the scope of this change
a behavioral regression in documentation fidelity.

I see, then I am going to audit colormap callers in the whole codebase to see whether updating colormap documentation is enough, or fixing mne.viz.plot_source_estimates to accept ndarray instead.

a deprecation notice or explicit error with a clear message would be more user-friendly...

I beg to politely differ, aligning documentation with the actual behaviour should be the priority, then, raising an extra error for a type that is not accepted seems redundant. Anyways, I would check the previous step to see what action is suitable.

Also, 13797.other.rst is missing a newline at end of file, which may cause formatting issues in the rendered changelog.

Got it, will fix it in the next commit.

Thanks again for your extensive thoughts, really appreciate it!

@Aniketsy
Copy link
Copy Markdown
Contributor

Also, 13797.other.rst is missing a newline at end of file, which may cause formatting issues in the rendered changelog.

@JiwaniZakir we don't need new line in changelog entry
Also my suggestion @Gnefil please wait for the review from maintainers if anything is unclear for you. Thanks!

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.

mne.viz.plot_source_estimates docs incorrectly say colormap can be supplied as an ndarray

5 participants