Add shading to Axes3D.voxels, and enable it by default

[eric-wieser]

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

• Has Pytest style unit tests
  • Do the existing ones count? Should I set shade=False on one of the existing tests, or parametrize them all on shaded vs not
• Code is Flake 8 compliant
  • Awaiting CI
• New features are documented, with examples if plot related
  • Do existing examples count?
• Documentation is sphinx and numpydoc compliant
• Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Is this major enough?
• Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way
  • Not backward-incompatible

[eric-wieser]

I assume pytest failures are related to #13122

[timhoffm]

I assume pytest failures are related to #13122

Yes, you can rebase now that this is in.

[eric-wieser]

Thanks @timhoffm, done

[timhoffm]

flake8 complains

./lib/mpl_toolkits/mplot3d/axes3d.py:2947:80: E501 line too long (83 > 79 characters)
1     E501 line too long (83 > 79 characters)

[NelleV]

This is really cool!
It needs a "what's new" entry (in users/next_whats_new/)
I also think we should update one of the examples from our gallery with this option. How about this one: https://matplotlib.org/devdocs/gallery/mplot3d/voxels.html#sphx-glr-gallery-mplot3d-voxels-py ?

[eric-wieser]

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.

[NelleV]

Sorry, I missed that.

That's a backward incompatible change on the design. @tacaswell do we still care about those?

[eric-wieser]

Yep, it is. That's the main thing I wanted feedback on

[anntzer]

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).
(Also note that the alternative would have been to have an even huger original PR that directly implemented shading, which would not have been optimal either :))

[dstansby]

I'm +/- 0 on changing the default without deprecating; either way this needs an API changes note, and possibly a what's new too.

[NelleV]

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)

[eric-wieser]

How was shading introduced for the other functions? Or was it always there for bar3d, trisurf, ...

[WeatherGod]

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> .

[anntzer]

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).

[ImportanceOfBeingErnest]

Concerning changing the defaults: With #12259 being merged, all 3d bar plots will now have a different shading than before. I don't think #12259 has any "what's new" or similar in it, and it also will break someone's image comparisson.

[eric-wieser]

And #12792 will have broken image comparison for all shaded 3d plots.

[efiring]

I'm convinced by the argument that it is OK to proceed with this change, since voxels are quite new, etc. ping @dstansby

(Y) to go ahead

[eric-wieser]

I'll try to add a whats-new entry today.

[eric-wieser]

What's new entry added (edit: And now works)

[eric-wieser]

Screwed up the plot a little, trying again

[eric-wieser]

Fixed plot: https://18441-1385122-gh.circle-artifacts.com/0/home/circleci/project/doc/build/html/users/next_whats_new/2019-01-06-shaded-voxels.html

[tacaswell]

I want to take a careful look at this and catch up on the conversation.

[tacaswell]

I think this is ok to change; it is a pretty clear improvement to a new feature.

I added a file to the next_api_changes so it will be noted there as well, anyone can merge once CI finishes again (just be paranoid about sphinx).