Provide `adjustable='box'` to 3D axes aspect ratio setting by tfpf · Pull Request #23552 · matplotlib/matplotlib · GitHub

PR Summary

#23409 implemented equal aspect for 3D axes similar to the wayaxes2d.set_aspect('equal', adjustable='datalim') works. This adds functionality similar to the way axes2d.set_aspect('equal', adjustable='box') works (i.e. without changing the limits on the axes of coordinates).

The code is probably not the cleanest; style suggestions are appreciated.

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

New features are documented, with examples if plot related.
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).

I think that it may be possible to factor out

            if aspect == 'equal':
                ax_indices = [0, 1, 2]
            elif aspect == 'equalxy':
                ax_indices = [0, 1]
            elif aspect == 'equalxz':
                ax_indices = [0, 2]
            elif aspect == 'equalyz':
                ax_indices = [1, 2]

and then do something like:

ax_indices = _get_ax_indices(aspect)
old_diag = np.norm([box_aspect[a] for a in ax_indices])  # Maybe better way to pick out values?
new_diag = np.norm([deltas[a] for a in ax_indices])  # Same
for i in ax_indices:
    box_aspect[i] = deltas[i]
remaining_index = {0, 1, 2} - set(ax_indices)
if remaining_index:
   box_aspect[remining_index.pop()] = new_diag/old_diag

(I take it that numpy may have a better way to do the index fiddling.)

Ahh, I didn't see the indentation level correctly. And it seems like you can get away with simpler indexing, something like:

old_diag = np.norm(box_aspect[ax_indices])
new_diag = np.norm(deltas[ax_indices])
box_aspect[ax_indices] = deltas[ax_indices]
remaining_index = {0, 1, 2} - set(ax_indices)
if remaining_index:
   box_aspect[remining_index.pop()] = new_diag/old_diag

Looks promising! I think a test, update of documentation (right now it says that it is ignored), a release note, and an argument check (similar to the aspect test, using _api.check_in_list) and one can review it properly.

For test, I guess that reusing the same aspect test, but with adjustable='box' may be a good and easy way.

I think that it may be possible to factor out

I've got this factored out here at the moment, we can settle on whatever.

Turns out that the variable was already available at that point in the code. Bad read-up on my side.

I think that it may be possible to factor out

I've got this factored out

This is brilliant; I'll rebase once it gets merged.

I think that it may be possible to factor out

I've got this factored out

This is brilliant; I'll rebase once it gets merged.

I expect that MR to take a while to get reviewed and go through so don't let it hold you back on this one! :) I can handle the merge conflict over there.

This is great! Looks good to me now.

Doc build has failed. I think this is the error (since the log says that 'build finished with problems, 1 warning'):

/home/circleci/project/doc/users/next_whats_new/font_fallback.rst:7: WARNING: Exception occurred in plotting font_fallback-1
 from /home/circleci/project/doc/users/next_whats_new/font_fallback.rst:
Traceback (most recent call last):
  File "/home/circleci/project/lib/matplotlib/sphinxext/plot_directive.py", line 644, in render_figures
    figman.canvas.figure.savefig(img.filename(fmt), dpi=dpi)
  File "/home/circleci/project/lib/matplotlib/figure.py", line 3197, in savefig
    self.canvas.print_figure(fname, **kwargs)
  File "/home/circleci/project/lib/matplotlib/backend_bases.py", line 2339, in print_figure
    result = print_method(
  File "/home/circleci/project/lib/matplotlib/backend_bases.py", line 2205, in <lambda>
    print_method = functools.wraps(meth)(lambda *args, **kwargs: meth(
  File "/home/circleci/project/lib/matplotlib/_api/deprecation.py", line 410, in wrapper
    return func(*inner_args, **inner_kwargs)
  File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 520, in print_png
    self._print_pil(filename_or_obj, "png", pil_kwargs, metadata)
  File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 466, in _print_pil
    FigureCanvasAgg.draw(self)
  File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 408, in draw
    self.figure.draw(self.renderer)
  File "/home/circleci/project/lib/matplotlib/artist.py", line 74, in draw_wrapper
    result = draw(artist, renderer, *args, **kwargs)
  File "/home/circleci/project/lib/matplotlib/artist.py", line 51, in draw_wrapper
    return draw(artist, renderer)
  File "/home/circleci/project/lib/matplotlib/figure.py", line 2985, in draw
    mimage._draw_list_compositing_images(
  File "/home/circleci/project/lib/matplotlib/image.py", line 131, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/circleci/project/lib/matplotlib/artist.py", line 51, in draw_wrapper
    return draw(artist, renderer)
  File "/home/circleci/project/lib/matplotlib/text.py", line 688, in draw
    bbox, info, descent = self._get_layout(renderer)
  File "/home/circleci/project/lib/matplotlib/text.py", line 322, in _get_layout
    w, h, d = _get_text_metrics_with_cache(
  File "/home/circleci/project/lib/matplotlib/text.py", line 97, in _get_text_metrics_with_cache
    return _get_text_metrics_with_cache_impl(
  File "/home/circleci/project/lib/matplotlib/text.py", line 105, in _get_text_metrics_with_cache_impl
    return renderer_ref().get_text_width_height_descent(text, fontprop, ismath)
  File "/home/circleci/project/lib/matplotlib/backends/backend_agg.py", line 242, in get_text_width_height_descent
    font.set_text(s, 0.0, flags=get_hinting_flag())
  File "/home/circleci/project/lib/matplotlib/_text_helpers.py", line 16, in warn_on_missing_glyph
    _api.warn_external(
  File "/home/circleci/project/lib/matplotlib/_api/__init__.py", line 363, in warn_external
    warnings.warn(message, category, stacklevel)
UserWarning: Glyph 20960 (\N{CJK UNIFIED IDEOGRAPH-51E0}) missing from current font.

which isn't quite clear to me. I don't suppose modifying 3d_plot_aspects.rst caused this somehow?

The docs font fallback issue is unrelated to your PR, try rebasing onto main and seeing if it works when it runs again.

It looks like this didn't get reviewed in time to make the cut for 3.6, which won't have an effect except that the what's new might need to be rewritten.

How does it work—will it be enough to just create a new "What's New" file?

Actually, no changes are required for the what's new. The content and structure is version agnostic. When the PR gets merged the content is put int the next_whats_new folder on the main branch, and that will be picked up in the next release.

@oscargus you have started looking into this. Could you give a second review?

Considering that this PR modifies a what's new note from 3.6, I believe that something should be done.

I think the PR is good to go, subject to the what's new note, so will not merge at this stage.

Could you also take a look at #23552 (comment).

For the "What's New" entry, I used scottshambaugh's original example, but with slightly different dimensions of the box (as in the test), which I think highlight the differences nicely.

Looks good! Thanks!

…b#23552) * Provided `adjustable='box'` option to set 3D aspect ratio. * "What's New": `adjustable` argument of 3D plots aspect ratio.

tfpf changed the title Aspect3d Provide adjustable='box' to 3D axes aspect ratio setting Aug 4, 2022

tfpf force-pushed the aspect3d branch from 1794023 to aa9592c Compare August 4, 2022 09:04

oscargus added the topic: mplot3d label Aug 4, 2022

oscargus reviewed Aug 4, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated

oscargus reviewed Aug 4, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py

oscargus reviewed Aug 4, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated

scottshambaugh reviewed Aug 4, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated

scottshambaugh reviewed Aug 4, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py

scottshambaugh reviewed Aug 4, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py Outdated

tfpf force-pushed the aspect3d branch 2 times, most recently from aa5228b to 2a702cf Compare August 5, 2022 07:52

tfpf commented Aug 6, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/mplot3d/axes3d.py

tfpf force-pushed the aspect3d branch from 21f9d54 to a4ea381 Compare August 7, 2022 11:39

oscargus reviewed Aug 7, 2022

View reviewed changes

Comment thread lib/mpl_toolkits/tests/test_mplot3d.py Outdated

tfpf force-pushed the aspect3d branch 3 times, most recently from d9518e0 to 75e605f Compare August 7, 2022 14:36

tfpf marked this pull request as ready for review August 7, 2022 14:36

scottshambaugh approved these changes Aug 7, 2022

View reviewed changes

scottshambaugh mentioned this pull request Aug 26, 2022

3d plot view angle documentation #23721

Merged

2 tasks done

oscargus approved these changes Aug 29, 2022

View reviewed changes

oscargus added this to the v3.7.0 milestone Aug 29, 2022

Provided adjustable='box' option to set 3D aspect ratio.

8941637

tfpf force-pushed the aspect3d branch 2 times, most recently from 76ab0f0 to 57f2b5a Compare August 29, 2022 18:33

"What's New": adjustable argument of 3D plots aspect ratio.

862ed02

tfpf force-pushed the aspect3d branch from 57f2b5a to 862ed02 Compare August 30, 2022 03:55

oscargus merged commit 7c6a74c into matplotlib:main Aug 30, 2022

tfpf deleted the aspect3d branch August 31, 2022 13:23

JeS24 mentioned this pull request Nov 4, 2022

Geodesic Plotting improvements einsteinpy/einsteinpy#624

Open

3 tasks

liuzhenqi77 mentioned this pull request Nov 17, 2022

Update aspect parameter in plot_point_brain netneurolab/netneurotools#122

Open

Provide `adjustable='box'` to 3D axes aspect ratio setting by tfpf · Pull Request #23552 · matplotlib/matplotlib · GitHub

Repository navigation

Provide adjustable='box' to 3D axes aspect ratio setting - #23552

Provide adjustable='box' to 3D axes aspect ratio setting#23552oscargus merged 2 commits intomatplotlib:mainmatplotlib/matplotlib:mainfrom tfpf:aspect3dtfpf/matplotlib:aspect3dCopy head branch name to clipboard

Conversation

tfpf commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

oscargus commented Aug 4, 2022

Uh oh!

oscargus commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oscargus commented Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scottshambaugh commented Aug 4, 2022

Uh oh!

Uh oh!

oscargus commented Aug 4, 2022

Uh oh!

tfpf commented Aug 4, 2022

Uh oh!

Uh oh!

Uh oh!

scottshambaugh commented Aug 4, 2022

Uh oh!

scottshambaugh commented Aug 5, 2022

Uh oh!

tfpf commented Aug 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

scottshambaugh commented Aug 6, 2022

Uh oh!

Uh oh!

scottshambaugh commented Aug 26, 2022

Uh oh!

tfpf commented Aug 29, 2022

Uh oh!

timhoffm commented Aug 29, 2022

Uh oh!

oscargus commented Aug 29, 2022

Uh oh!

tfpf commented Aug 29, 2022

Uh oh!

tfpf commented Aug 29, 2022

Uh oh!

oscargus commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Provide adjustable='box' to 3D axes aspect ratio setting#23552
oscargus merged 2 commits into
matplotlib:mainfrom
tfpf:aspect3d

tfpf commented Aug 4, 2022 •
edited
Loading

oscargus commented Aug 4, 2022 •
edited
Loading

tfpf commented Aug 6, 2022 •
edited
Loading