Skip to content

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

Merged
QuLogic merged 2 commits into
matplotlib:masterfrom
eric-wieser:voxels-shading
Feb 23, 2019
Merged

Add shading to Axes3D.voxels, and enable it by default#13123
QuLogic merged 2 commits into
matplotlib:masterfrom
eric-wieser:voxels-shading

Conversation

@eric-wieser

@eric-wieser eric-wieser commented Jan 6, 2019

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

I assume pytest failures are related to #13122

@timhoffm

timhoffm commented Jan 7, 2019

Copy link
Copy Markdown
Member

I assume pytest failures are related to #13122

Yes, you can rebase now that this is in.

@eric-wieser

Copy link
Copy Markdown
Contributor Author

Thanks @timhoffm, done

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py
@timhoffm

timhoffm commented Jan 7, 2019

Copy link
Copy Markdown
Member

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

NelleV commented Jan 18, 2019

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

NelleV commented Jan 18, 2019

Copy link
Copy Markdown
Member

Sorry, I missed that.

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

@eric-wieser

Copy link
Copy Markdown
Contributor Author

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

@anntzer

anntzer commented Jan 18, 2019

Copy link
Copy Markdown
Contributor

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 dstansby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

NelleV commented Jan 21, 2019

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

@WeatherGod

WeatherGod commented Jan 21, 2019 via email

Copy link
Copy Markdown
Member

@anntzer anntzer added this to the v3.1.0 milestone Feb 4, 2019
@anntzer

anntzer commented Feb 4, 2019

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

eric-wieser commented Feb 5, 2019

Copy link
Copy Markdown
Contributor Author

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

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 11, 2019
@efiring

efiring commented Feb 11, 2019

Copy link
Copy Markdown
Member

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

@eric-wieser

Copy link
Copy Markdown
Contributor Author

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

@eric-wieser

eric-wieser commented Feb 17, 2019

Copy link
Copy Markdown
Contributor Author

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

@eric-wieser eric-wieser force-pushed the voxels-shading branch 2 times, most recently from f6ee9a0 to e568241 Compare February 17, 2019 23:18
@eric-wieser

Copy link
Copy Markdown
Contributor Author

Screwed up the plot a little, trying again

This also required slightly more care about face ordering, hence the extra square variables.
@eric-wieser

Copy link
Copy Markdown
Contributor Author

@tacaswell tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@tacaswell

Copy link
Copy Markdown
Member

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

@QuLogic QuLogic merged commit 7f350d3 into matplotlib:master Feb 23, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 23, 2019
dstansby added a commit that referenced this pull request Feb 23, 2019
…123-on-v3.1.x

Backport PR #13123 on branch v3.1.x (Add shading to Axes3D.voxels, and enable it by default)
@eric-wieser eric-wieser deleted the voxels-shading branch March 23, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: mplot3d

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants