NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

Fix array-like linewidth for 3d scatter#23434

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

Jump to bottom

Merged

tacaswell merged 4 commits intomatplotlib:mainfromkrassowski:fix-23433

Aug 3, 2022

Merged

Fix array-like linewidth for 3d scatter#23434

tacaswell merged 4 commits intomatplotlib:mainfromkrassowski:fix-23433

Aug 3, 2022

Conversation

Copy link

Contributor

krassowski commentedJul 16, 2022

PR Summary

Fixes#23433 by castingself._linewidths3d tonp.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 inset_linewidth and inset_3d_properties, but I went for the simplest solution possible. Let me know if you want to change that (in that case, isself._linewidth3d = lw a typo? should it be_linewidths3d?)

PR Checklist

Tests and Styling

Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

Fix array-like linewidth for 3d scatter

0076141

github-actionsbot reviewed

Jul 16, 2022

View reviewed changes

Copy link

github-actionsbot left a comment

Choose a reason for hiding this comment

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

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 uson gitter for real-time discussion.

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

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

Copy link

Member

QuLogic commentedJul 17, 2022

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.

Cast to np.array earlier (inset_3d_properties)

f88d1f6

oscargus approved these changes

Jul 19, 2022

View reviewed changes

Copy link

Member

oscargus commentedJul 20, 2022

I think this should be changed as well:

matplotlib/lib/mpl_toolkits/mplot3d/art3d.py

Lines 563 to 566 in3992fb0

	defset_linewidth(self,lw):
	super().set_linewidth(lw)
	ifnotself._in_draw:
	self._linewidth3d=lw

Both changing the name from_linewidth3d to_linewidths3d and converting into an np-array.

Copy link

Member

oscargus commentedJul 20, 2022•
edited
Loading

Maybe one can also add acheck_figures_equal test that uses theset_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 the topic: mplot3d label

Jul 20, 2022

oscargus added this to thev3.6.0 milestone

Jul 20, 2022

Fix typo, ensure thatlinewidths broadcasted in parent's method are…

261670f

… used

Copy link

ContributorAuthor

krassowski commentedJul 22, 2022•
edited
Loading

Maybe one can also add acheck_figures_equal test that uses theset_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, sinceset_linewidths is already there in the test case that you are pointing to (and sinceself._linewidths was already getting properly set in thesuper() 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.

Copy link

Member

oscargus commentedJul 22, 2022•
edited
Loading

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

Copy link

ContributorAuthor

krassowski commentedJul 24, 2022

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

Copy link

Contributor

scottshambaugh commentedJul 25, 2022•
edited
Loading

@krassowski looking atthe 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 agit commit --amend followed by agit push -f I believe that should change the timestamp on your last commit and cause the CI tests to rerun.