Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix IndexError when using scatter3d and depthshade=False#18156
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @mpl3d_image_comparison(['scatter3d_depthshade_false.png']) | ||
| def test_scatter3d_depthshade_false(): | ||
| """ | ||
| Test that 3d scatter plot works with depthshade=False (issue #18037) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We try to be stingy with our image tests. If this is just a test to see if depthshade=False works then maybe if doesn't need the image comparison (faster, takes less disk space etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll work on changing this. Thanks for the feedback!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
... you are allowed to claim it does need an image test, but no need to include one if its not necessary...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this might need an image test. I can't think of another way to generate a baseline figure without using the method being tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can always generate a figure, but not check the image output (just don't add thempl3d_image_comparison decorator). Since the original bug resulted in a traceback, that would be enough to test that the bug is fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@neilZon You don't need to compare figures either. You can just do
x=y=z=np.arange(16)ax_test=fig_test.add_subplot(projection='3d')ax_test.scatter(x,y,z,depthshade=False)
and the figure will get made. If the code you are testing throws an error, the test will fail. If not, then it will pass. We call this a "smoketest" because it just checks that the code runs, and nothing else. But bear in mind this test would fail on master right now, so its still a worthwhile (and quick!) test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh ok, I understand now. Thank you for the guidance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can't seem to get my code to fail now with this test
This looks fine to me. And thanks for adding a test, though see the note above! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the PR! The fix looks good to me. For the test, please could you remove the test image, and I don't think a figure comparison needs to be done. It is fine just to check that the code runs without producing an error, so thecheck_figures_equal can be removed, and you only have to create one figure/axes to check the code works.
Squash merging to make sure the image doesn't get committed |
…156-on-v3.3.xBackport PR#18156 on branch v3.3.x (Fix IndexError when using scatter3d and depthshade=False)
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Minor bug fix for issue#18037 that resulted in an IndexError when plotting more than 2 points on 3d scatter plot with depthshade=False
PR Checklist