Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Remove comment about cmap from voxels docstring#24284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tacaswell merged 1 commit intomatplotlib:mainfromksunden:voxels_docstring
Oct 27, 2022

Conversation

ksunden
Copy link
Member

PR Summary

voxels does not usecmap at all, and thus does not modifyshade in relation tocmap being passed in, this appears to be copy/pasted from 3D methods whichdo usecmap.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

voxels does not use cmap at all, and thus does not modify shade in relation to cmap being passed in, this appears to be copy/pasted from methods which _do_ use cmap
@story645story645 added this to thev3.6.2 milestoneOct 27, 2022
@tacaswell
Copy link
Member

It does not fall through to the underlyingPolyCollection3D?

@story645
Copy link
Member

I think that's part of what@oscargus is trying to do in#23914?

@ksunden
Copy link
MemberAuthor

@tacaswell Poly3DCollection makes no references to cmaps whatsoever, relying on the higher level functions to call the map to get facecolors.

In fact the light source shading itself is even done invoxels as well.

The data for voxels is fundamentally binary (show this voxel or don't) and so don't map onto a cmap.

Granted, thecmap kwarg is passed down the chain all the way toCollection and it is not an exception to pass one, but there is not data to evaluate that cmapat. voxels does not accept ac orcolors argument (though that may be a reasonable addition which would make the handling described by the current docstring necessary)

For reference, I noticed this while looking into#24277, where the reporter is requesting pretty much exactly that. its easy enough to work around with the tools provided, and bringing in norms and all of the color-focused arguments may be more effort than we wish.

@tacaswell
Copy link
Member

This is another candidate forremove_method?

@tacaswelltacaswell merged commit6ed3ab1 intomatplotlib:mainOct 27, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestOct 27, 2022
@ksundenksunden deleted the voxels_docstring branchOctober 28, 2022 00:07
QuLogic added a commit that referenced this pull requestOct 28, 2022
…284-on-v3.6.xBackport PR#24284 on branch v3.6.x (Remove comment about cmap from voxels docstring)
@ksundenksunden mentioned this pull requestFeb 20, 2023
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@story645story645story645 approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.2
Development

Successfully merging this pull request may close these issues.

4 participants
@ksunden@tacaswell@story645@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp