Skip to content

Improve mpl_toolkit documentation#23536

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
oscargus:mpltoolkitdocs
Oct 21, 2022
Merged

Improve mpl_toolkit documentation#23536
QuLogic merged 1 commit into
matplotlib:mainfrom
oscargus:mpltoolkitdocs

Conversation

@oscargus

@oscargus oscargus commented Aug 1, 2022

Copy link
Copy Markdown
Member

PR Summary

A bit of links and numpydocifying.

(Probably won't work at the first attempt as I cannot build locally...)

And a minor change in the code for efficiency.

PR Checklist

Tests and Styling

  • [N/A] 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).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the mpltoolkitdocs branch 6 times, most recently from 0f066c3 to cc71611 Compare August 2, 2022 10:21
@oscargus oscargus marked this pull request as ready for review August 2, 2022 10:21

@QuLogic QuLogic 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.

BTW, haven't commented everywhere, but I think there's no need to do ~.ClassName, as the ~ trims the displayed name as specified, so there's nothing to trim. It'd only have an effect if you did, e.g., ~.module.ClassName.

Comment thread lib/mpl_toolkits/axisartist/axis_artist.py Outdated
Comment thread lib/mpl_toolkits/axisartist/axis_artist.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py
Comment thread lib/mpl_toolkits/mplot3d/art3d.py
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/axis3d.py Outdated
Comment thread lib/mpl_toolkits/axes_grid1/axes_rgb.py Outdated
@oscargus oscargus marked this pull request as draft August 8, 2022 22:23
@oscargus oscargus force-pushed the mpltoolkitdocs branch 4 times, most recently from e88ef90 to 14b3aaf Compare August 17, 2022 09:15
@oscargus oscargus marked this pull request as ready for review August 17, 2022 09:17
@oscargus

Copy link
Copy Markdown
Member Author

I am a bit confused if one should keep the documentation for mpl_toolkit "separated" from matplotlib, i.e., if one should use full descriptors matplotlib.axes.Axes to show that it is a matplotlib class or ~.axes.Axes for brevity...

@timhoffm

Copy link
Copy Markdown
Member

I don't think we need extra separation when referencing types in mpl_toolkits. So https://matplotlib.org/stable/devel/documenting_mpl.html#referencing-types still applies.

@oscargus

Copy link
Copy Markdown
Member Author

In #19572 you, @timhoffm , suggested to make set_3d_properties private. Is that still your standpoint?

There's also #1482 and partly #1483 which may be relevant to consider.

@timhoffm

Copy link
Copy Markdown
Member

In #19572 you, @timhoffm , suggested to make set_3d_properties private. Is that still your standpoint?

Yes.

  • It should be an implementation detail that our 3D artists are 2D artists with a third dimension glued on. set_3d_properties leaks this concept. Having this public would make it much more difficult to replace Artists later with proper 3D Artists, in case we see the need for it.
  • The API is confusing: I can update x and y with one function, but have to use another function for z. That's weird. - Problem: We do not have actual 3D data setters in many cases. set_3d_properties is currently the only way. Before we can privatize it, we need a proper 3D set_data API.
  • The API is inconsistent between different Artists. Migrating this API to a homogeneous set_3d_properties interface is likely at least as much work as deprecating and providing set_data.

Comment thread lib/mpl_toolkits/axisartist/axis_artist.py Outdated
Comment thread lib/mpl_toolkits/axes_grid1/axes_divider.py Outdated
Comment thread lib/mpl_toolkits/axes_grid1/axes_divider.py Outdated
Comment thread lib/mpl_toolkits/axisartist/axis_artist.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
@oscargus oscargus force-pushed the mpltoolkitdocs branch 4 times, most recently from 342d48d to b95b4ab Compare October 15, 2022 22:01

@timhoffm timhoffm 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.

There are code cleanups in here, which makes this require 2 approvals.

Comment thread lib/mpl_toolkits/axes_grid1/axes_divider.py
Comment thread lib/mpl_toolkits/axes_grid1/axes_rgb.py Outdated
Comment thread lib/mpl_toolkits/axes_grid1/axes_rgb.py Outdated
Comment thread lib/mpl_toolkits/axes_grid1/axes_rgb.py Outdated
Comment thread lib/mpl_toolkits/axes_grid1/axes_rgb.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py
Comment thread lib/mpl_toolkits/mplot3d/art3d.py
Comment thread lib/mpl_toolkits/mplot3d/art3d.py
Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/art3d.py Outdated
Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated
@oscargus oscargus force-pushed the mpltoolkitdocs branch 2 times, most recently from 6c09ea0 to 1b4f2f7 Compare October 21, 2022 10:16
@QuLogic QuLogic added this to the v3.7.0 milestone Oct 21, 2022
@QuLogic QuLogic merged commit c450aa7 into matplotlib:main Oct 21, 2022
@oscargus oscargus deleted the mpltoolkitdocs branch October 22, 2022 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants