FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix array-like linewidth for 3d scatter by krassowski · Pull Request #23434 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fix array-like linewidth for 3d scatter - #23434

Merged
tacaswell merged 4 commits into
matplotlib:mainfrom
krassowski:fix-23433
Aug 3, 2022
ConversationCommits (4)ChecksFiles changed
Merged

Fix array-like linewidth for 3d scatter#23434
tacaswell merged 4 commits into
matplotlib:mainfrom
krassowski:fix-23433

Conversation

krassowski commented Jul 16, 2022

Copy link
Copy Markdown
Contributor

PR Summary

Fixes #23433 by casting self._linewidths3d to np.array at the latest stage possible and adds a test to ensure it works.

The casting step could alternatively be moved to happen earlier in the code logic in set_linewidth and in set_3d_properties, but I went for the simplest solution possible. Let me know if you want to change that (in that case, is self._linewidth3d = lw a typo? should it be _linewidths3d?)

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions Bot reviewed Jul 16, 2022
View reviewed changes

github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

QuLogic commented Jul 17, 2022

Copy link
Copy Markdown
Member

I think it'd be better to not do this so late, since this is in the draw call. Then the array can be done once instead of every refresh, which could be a bit slow.

oscargus commented Jul 20, 2022

Copy link
Copy Markdown
Member

I think this should be changed as well:

matplotlib/lib/mpl_toolkits/mplot3d/art3d.py

Lines 563 to 566 in 3992fb0

def set_linewidth(self, lw):
super().set_linewidth(lw)
if not self._in_draw:
self._linewidth3d = lw

Both changing the name from _linewidth3d to _linewidths3d and converting into an np-array.

oscargus commented Jul 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Maybe one can also add a check_figures_equal test that uses the set_linewidth method for one of the figures and the keyword argument for the other?

(See the test just below the one you added to see how they work.)

oscargus added this to the v3.6.0 milestone Jul 20, 2022

krassowski commented Jul 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Maybe one can also add a check_figures_equal test that uses the set_linewidth method for one of the figures and the keyword argument for the other?

(See the test just below the one you added to see how they work.)

To clarify, since set_linewidths is already there in the test case that you are pointing to (and since self._linewidths was already getting properly set in the super() call it worked ok for scalars as tested), would you like me to add another test like that, but testing that array-likes can be set via keyword argument and setter?

Edit: added, can remove if not needed.

oscargus commented Jul 22, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Considering the spelling error, I do not think that it was actually properly tested, so ideally there should be a test that does set_linewidth and relies on that the spelling is correct.

(The spelling error was introduced in #19812 btw.)

krassowski commented Jul 24, 2022

Copy link
Copy Markdown
Contributor Author

The test which I added in 2bdbdca should cover that. The documentation build seems to be failing, is there anything I should do, or is it intermittent CI issue?

scottshambaugh commented Jul 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

@krassowski looking at the docs build logs, I think this was just an intermittent CI issue. I don't think there's a way to re-trigger the tests through the github gui, but if you do a git commit --amend followed by a git push -f I believe that should change the timestamp on your last commit and cause the CI tests to rerun.

krassowski commented Jul 25, 2022

Copy link
Copy Markdown
Contributor Author

Thanks, all checks have passed now 🎉

krassowski commented Aug 2, 2022

Copy link
Copy Markdown
Contributor Author

A gentle ping on this PR :)

tacaswell merged commit 16ad86e into matplotlib:main Aug 3, 2022

tacaswell commented Aug 3, 2022

Copy link
Copy Markdown
Member

Congratulations on your first merged Matplotlib PR @krassowski 🎉 Hopefully we will hear from you again!

Also thank you for the gentle ping!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

github-actions[bot] github-actions[bot] left review comments
oscargus oscargus approved these changes

Assignees

No one assigned

Labels

topic: mplot3d

Projects

First Time Contributors
Status: Merged

Milestone

v3.6.0

Development

Successfully merging this pull request may close these issues.

[Bug]: array-like linewidth raises an error for scatter3D

5 participants

Back | FazBrowse Home | New Git URL