Add shading to Axes3D.voxels, and enable it by default #13123
Conversation
|
I assume pytest failures are related to #13122 |
Yes, you can rebase now that this is in. |
3cea6a4 to
4e6c2d8
Compare
|
Thanks @timhoffm, done |
|
flake8 complains |
4e6c2d8 to
d1a4f5a
Compare
|
This is really cool! |
|
Shading is now the default - all of those gallery examples will use it now. I'll add the what's new when I get around to it. |
|
Sorry, I missed that. That's a backward incompatible change on the design. @tacaswell do we still care about those? |
|
Yep, it is. That's the main thing I wanted feedback on |
|
Given that the function is relatively recent (2.1 i.e. October 2017) I think its is reasonable to allow the change without a deprecation period (needs an API change note though). |
|
I'm -0.5 on changing the defaults (we regularly break people's code by changing the defaults, including libraries that depend on Matplotlib and perform images comparison tests) |
|
How was shading introduced for the other functions? Or was it always there for bar3d, trisurf, ... |
|
it was always there
…On Sun, Jan 20, 2019 at 10:30 PM Eric Wieser ***@***.***> wrote:
How was shading introduced for the other functions? Or was it always there
for bar3d, trisurf, ...
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#13123 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AARy-AR792svgwk354jiAYtwt7yGN-XWks5vFTRPgaJpZM4Zylrv>
.
|
|
Milestoning as 3.1 so that we can at least reach a decision of whether to change the default there or not (the longer we wait, the less reasonable it is to change the default without a deprecation period). |
|
And #12792 will have broken image comparison for all shaded 3d plots. |
|
I'm convinced by the argument that it is OK to proceed with this change, since voxels are quite new, etc. ping @dstansby |
|
I'll try to add a whats-new entry today. |
d1a4f5a to
4d23d86
Compare
|
What's new entry added (edit: And now works) |
f6ee9a0 to
e568241
Compare
|
Screwed up the plot a little, trying again |
This also required slightly more care about face ordering, hence the extra square variables.
e568241 to
6c81208
Compare
|
I think this is ok to change; it is a pretty clear improvement to a new feature. I added a file to the |
…e it by default
…123-on-v3.1.x Backport PR #13123 on branch v3.1.x (Add shading to Axes3D.voxels, and enable it by default)
PR Summary
The conclusion of my normals and shading work (#12792, #12136 ).
The rich image diffs in the patch should show the effect. For a quick summary, here's what the output now looks like:
PR Checklist
shade=Falseon one of the existing tests, or parametrize them all on shaded vs not