Skip to content

do not ignore "closed" parameter in Poly3DCollection #8014

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 6, 2017
Merged

do not ignore "closed" parameter in Poly3DCollection #8014

merged 2 commits into from
Feb 6, 2017

Conversation

dlaidig
Copy link
Contributor

@dlaidig dlaidig commented Feb 3, 2017

In Poly3DCollection the "closed" parameter is effectively ignored, as Poly3DCollection.do_3d_projection will call PolyCollection.set_verts without passing the parameter, leading to the polygons always being closed.

To fix this, I store the "closed" parameter in Poly3DCollection.set_verts and apply it in do_3d_projection. Passing closed=True to PolyCollection in Poly3DCollection.set_verts only seems to add overhead, so I changed it to always pass False.

Example demonstrating the bug:

import numpy as np
import matplotlib.pyplot as plt
import mpl_toolkits.mplot3d as a3
from mpl_toolkits.mplot3d.art3d import Poly3DCollection

fig = plt.figure()
ax = fig.gca(projection='3d')
poly1 = np.array([[0, 0, 1], [0, 1, 1], [0, 0, 0]], np.float)
poly2 = np.array([[0, 1, 1], [1, 1, 1], [1, 1, 0]], np.float)
collection1 = Poly3DCollection([poly1], linewidths=3, edgecolor='k', facecolor=(0.5, 0.5, 1, 0.5), closed=True)
collection2 = Poly3DCollection([poly2], linewidths=3, edgecolor='k', facecolor=(1, 0.5, 0.5, 0.5), closed=False)
ax.add_collection3d(collection1)
ax.add_collection3d(collection2)
plt.savefig('poly3d.png')

I do not have any experience with the matplotlib internals, so please bear with me in case I got something wrong.

@dlaidig
Copy link
Contributor Author

dlaidig commented Feb 3, 2017

test_mixedsubplots does not fail on my system...

@tacaswell
Copy link
Member

That test has some non-deterministic element and is known to be flaky.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 3, 2017
@tacaswell tacaswell requested a review from WeatherGod February 3, 2017 13:29
@tacaswell
Copy link
Member

Provisional 👍 from me, leave it to @WeatherGod to decide if this needs an image test or not.

@WeatherGod
Copy link
Member

This seems to work. We used to have major problems with the closed/unclosed polygons that we just went ahead and forced everything to be closed in order to get the trisurf stuff working and fix a few other bugs. I am a little surprised that the fix is as simple as it is.

The only thing we need is for the posted example be made into an image test. Tests are in lib/mpl_toolkits/tests/test_mplot3d.py and images go into lib/mpl_toolkits/tests/baseline_images/test_mplot3d/.

@WeatherGod WeatherGod merged commit c4b8fe6 into matplotlib:master Feb 6, 2017
@WeatherGod
Copy link
Member

Thank you for your fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WeatherGod WeatherGod Awaiting requested review from WeatherGod

Assignees
No one assigned
Labels
topic: mplot3d
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
4 participants
@dlaidig @tacaswell @WeatherGod @dstansby