Do not add padding to 3D axis limits when limits are manually set #25272

scottshambaugh · 2023-02-21T07:13:13Z

PR Summary

import matplotlib.pyplot as plt
fig, ax = plt.subplots(1, 1, subplot_kw={'projection': '3d'})
ax.set_xlim(0, 0.8)
ax.set_ylim(0, 0.8)
ax.set(xlabel='x', ylabel='y', zlabel='z')
plt.show()

Before:

After (exact limits for x and y, z retains default padded limits):

~~There are a ton of test image changes, gonna be a hefty commit.~~

PR Checklist

Documentation and Tests

Has pytest style unit tests (and pytest passes)
Documentation is sphinx and numpydoc compliant (the docs should build without error).
[N/A] New plotting related features are documented with examples.

Release Notes

[N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
[N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
[N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

oscargus · 2023-02-21T07:30:21Z

Although I think that this is really how it should be, the question is if we can just enforce it or if there should be a deprecation path? Better ping in @timhoffm directly...

scottshambaugh · 2023-02-21T08:07:19Z

This is changing intended behavior in some of the tests so some tweaking to do, but should be enough here right now to get an idea.

scottshambaugh · 2023-02-24T01:27:29Z

Talked about this in the dev call today, consensus was to make the new behavior default without deprecation, but add an rc_param to switch back to current behavior.

scottshambaugh · 2023-04-30T03:27:43Z

This is ready for review!

Note that there are a large number (57) of baseline image edits. In all cases, these are very minor - either a 1 pixel shift in some labels, or a subpixel shift in tick locations. I believe these are likely due to accumulated floating point errors as a result of several scale conversion factors that I had to add to match the baseline formatting.

Take for example axes3d_labelpad-failed-diff.png:

Which when thresholded to maximum contrast in photoshop shows some slight subpixel shifts for the tick marks:

scottshambaugh · 2023-04-30T04:19:32Z

Well, it looks like the floating point theory is likely correct, because the tests are all passing on my machine, and the different CI platforms are having different tests throw image comparison errors. What's the proper way to handle this? Loosen the image comparison tolerance? I'm not sure how to examine the test images from the different CI environments if I can't recreate the issue locally.

oscargus · 2023-04-30T12:22:05Z

The ideal way is to add platform-specific margins, like:

matplotlib/lib/matplotlib/tests/test_lines.py

Lines 187 to 189 in e7fd79f

    
           @image_comparison( 
        
               ['line_collection_dashes'], remove_text=True, style='mpl20', 
        
               tol=0.65 if platform.machine() in ('aarch64', 'ppc64le', 's390x') else 0)

I think one way to make it at least slightly better is to pre-compute the constants. In this way one error source is removed and the code will execute slightly faster. Always possible to use a comment to illustrate where it comes from.

oscargus

Most of the constants should be pre-computed I think.

Doc-strings should typically have a single-sentence/line description (hence, the need for an empty line after).

A change note is required for the new rcParam

I think the purpose of the PR is for sure worthwhile, but I do not know enough to have any real comments on the implentation.

lib/matplotlib/axis.py

lib/matplotlib/testing/decorators.py

lib/matplotlib/ticker.py

lib/mpl_toolkits/mplot3d/axes3d.py

lib/mpl_toolkits/mplot3d/axis3d.py

lib/mpl_toolkits/mplot3d/axes3d.py

lib/matplotlib/axis.py

lib/mpl_toolkits/mplot3d/axes3d.py

lib/matplotlib/mpl-data/matplotlibrc

lib/mpl_toolkits/mplot3d/axes3d.py

scottshambaugh · 2023-04-30T20:55:16Z

Thank you for the thorough review @oscargus!

To give some context for where these constants are coming from, the original behavior when generating the view limits for 3d plots was fairly broken. There was always 1/48 of the axis range added to either side of the axis bounds when drawing the axes, however this extra "view margin" was purely visual. The calculations for the zoom level for the plot, which grid/tick lines to show, and several other spots in the code all used the non-padded axis limits for their calculation. So, when I converted the backed to use the actual axis limits everywhere, this required putting in a lot of scale factors to keep the visual appearance (23/24 = 1-2*1/48, and 25/24 = 1+2*1/48).

tacaswell · 2023-08-02T20:05:56Z

@scottshambaugh this needs a rebase because we merged some other PRs

tacaswell · 2023-08-02T20:11:59Z

Talking about this with @ksunden and @QuLogic we want to take this, but given this is an API change and it is the 11th hour of the 3.8 prep, we are going to hold this for 3.9 (which means it should get merged as soon we branch).

QuLogic · 2023-08-02T20:12:10Z

Essentially - the old padding was 1/48, and was applied 2x (to the min and max limits), so that's where the 1 ± 1/24 scaling came from.

OK, can we mention that in the comments? It currently says something like "to match mpl3.7 appearance", so it can say "to match mpl3.7 appearance that added 1/48 padding". Also feel free to put a longer explanation in the commit message.

scottshambaugh · 2023-08-03T01:36:32Z

Thanks! Rebased/squashed, and added a lot more detail to the comments on scale factors. @QuLogic do those address your concerns?

Fix remaining large image diffs Fix remaining large image diffs Fix the rest of the tests Fix docs error Code review updates Code review updates Fix docs error One more constant Tweaks Keep fixing CI errors Docstrings Update baseline images More test images Images Test new scaling factor Test new scaling factor Test for exact limits Cleanup Tests Update doc/users/next_whats_new/3d_axis_limits.rst Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com> rebase fixes Apply suggestions from code review Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com> Code review cleanup Merge conflicts and better comments Merge conflicts and better comments

scottshambaugh · 2023-09-17T23:33:42Z

Bump on merging this now that 3.8.0 is out

timhoffm · 2023-09-18T15:51:47Z

@scottshambaugh Thanks!

ksunden · 2023-09-18T17:04:22Z

Unclear to me why this didn't happen for this PR, but references to [xy]axis_inverted and invert_[xy]axis in the docstrings from this PR are causing doc build failures on main now:

https://app.circleci.com/pipelines/github/matplotlib/matplotlib/26910/workflows/8053f52d-d0f9-45f0-8b9e-e759ea3c4215/jobs/78885

/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.get_xbound:20: WARNING: py:obj reference target not found: invert_xaxis
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.get_xbound:20: WARNING: py:obj reference target not found: xaxis_inverted
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.get_ybound:20: WARNING: py:obj reference target not found: invert_yaxis
/home/circleci/project/lib/matplotlib/axes/_base.py:docstring of matplotlib.axes._base._AxesBase.get_ybound:20: WARNING: py:obj reference target not found: yaxis_inverted
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_xbound:31: WARNING: py:obj reference target not found: invert_xaxis
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_xbound:31: WARNING: py:obj reference target not found: xaxis_inverted
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_xlim:53: WARNING: py:obj reference target not found: invert_xaxis
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_xlim:53: WARNING: py:obj reference target not found: xaxis_inverted
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_ybound:31: WARNING: py:obj reference target not found: invert_yaxis
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_ybound:31: WARNING: py:obj reference target not found: yaxis_inverted
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_ylim:53: WARNING: py:obj reference target not found: invert_yaxis
/home/circleci/project/lib/mpl_toolkits/mplot3d/axes3d.py:docstring of mpl_toolkits.mplot3d.axes3d.Axes3D.set_ylim:53: WARNING: py:obj reference target not found: yaxis_inverted

It looks like the warning is that it can't find them at all, rather than that it e.g. sees both Axes.invert_yaxis and Axes3D.invert_yaxis, which was my first thought.

Sphinx version is different (7.2.4 here, 7.2.6 on main), though only as patch releases, and nothing obviously related in release notes.

mpl-sphinx-theme version is different, though I can't think of a way that should affect this linking behavior.

Attempt to fix doc build failure introduced with matplotlib#25272. I suspect this is sphinx scoping: matplotlib#25272 reused methods like `Axes.set_xbounds` to generate documentation for Axes3D. However, a see also of invert_xaxis only searches within the same module (or class), and invert_xaxis is not documented for Axes3D. This PR broadens the search scope by using the leading dot. An alternative may be to attempt to explicitly document the missing methods for Axes3D.

Attempt to fix doc build failure introduced with matplotlib#25272. I suspect this is sphinx scoping: matplotlib#25272 reused methods like `Axes.set_xbounds` to generate documentation for Axes3D. However, a see also of invert_xaxis only searches within the same module (or class), and invert_xaxis is not documented for Axes3D. This PR attempt to explicitly document the missing methods for Axes3D.

scottshambaugh marked this pull request as draft February 21, 2023 07:15

scottshambaugh force-pushed the 3d_exact_limits branch from 56f5c0e to eb13ba8 Compare February 21, 2023 07:42

oscargus added topic: mplot3d status: needs comment/discussion needs consensus on next step labels Feb 21, 2023

scottshambaugh removed the status: needs comment/discussion needs consensus on next step label Feb 24, 2023

scottshambaugh force-pushed the 3d_exact_limits branch from ba17fca to 164a811 Compare February 25, 2023 01:50

github-actions bot added the status: needs rebase label Mar 8, 2023

scottshambaugh force-pushed the 3d_exact_limits branch from 164a811 to 9ed0c5b Compare March 13, 2023 00:52

github-actions bot removed the status: needs rebase label Mar 13, 2023

github-actions bot added status: needs rebase and removed status: needs rebase labels Apr 29, 2023

scottshambaugh force-pushed the 3d_exact_limits branch 3 times, most recently from 3200e54 to 2239207 Compare April 29, 2023 23:28

scottshambaugh marked this pull request as ready for review April 30, 2023 03:27

scottshambaugh force-pushed the 3d_exact_limits branch from 40fc8f0 to 1c04693 Compare April 30, 2023 03:35

scottshambaugh force-pushed the 3d_exact_limits branch from 00f03ca to 43f7d59 Compare April 30, 2023 05:44

oscargus reviewed Apr 30, 2023

View reviewed changes

lib/matplotlib/mpl-data/matplotlibrc Show resolved Hide resolved

oscargus reviewed Apr 30, 2023

View reviewed changes

lib/mpl_toolkits/mplot3d/axes3d.py Outdated Show resolved Hide resolved

scottshambaugh force-pushed the 3d_exact_limits branch from b55d12c to 978889b Compare April 30, 2023 20:45

scottshambaugh marked this pull request as draft April 30, 2023 20:56

scottshambaugh force-pushed the 3d_exact_limits branch from 978889b to 415763e Compare April 30, 2023 21:44

scottshambaugh added this to the v3.8.0 milestone Jul 11, 2023

tacaswell modified the milestones: v3.8.0, v3.9.0 Aug 2, 2023

github-actions bot added the status: needs rebase label Aug 2, 2023

scottshambaugh force-pushed the 3d_exact_limits branch from 6b38c5e to dfdc014 Compare August 3, 2023 00:23

github-actions bot removed the status: needs rebase label Aug 3, 2023

scottshambaugh force-pushed the 3d_exact_limits branch 2 times, most recently from fe42617 to 24254de Compare August 3, 2023 01:08

github-actions bot added the status: needs rebase label Aug 9, 2023

scottshambaugh force-pushed the 3d_exact_limits branch from 24254de to f1141d1 Compare August 28, 2023 21:30

github-actions bot removed the status: needs rebase label Aug 28, 2023

scottshambaugh force-pushed the 3d_exact_limits branch 2 times, most recently from f60f5a0 to 0da1b6c Compare August 28, 2023 22:08

oscargus approved these changes Sep 18, 2023

View reviewed changes

timhoffm approved these changes Sep 18, 2023

View reviewed changes

timhoffm merged commit 8f3e9e6 into matplotlib:main Sep 18, 2023
40 checks passed

This was referenced Sep 18, 2023

Fix doc build #26817

Closed

Fix doc build (alternative) #26818

Merged

scottshambaugh mentioned this pull request Nov 21, 2023

[DOC]: Update 3d axis limits what's new #27357

Merged

1 task

QuLogic mentioned this pull request Apr 24, 2024

Test failure with Matplotlib 3.9.0rc2 boutproject/animatplot-ng#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not add padding to 3D axis limits when limits are manually set #25272

Do not add padding to 3D axis limits when limits are manually set #25272

scottshambaugh commented Feb 21, 2023 •

edited

oscargus commented Feb 21, 2023

scottshambaugh commented Feb 21, 2023

scottshambaugh commented Feb 24, 2023

scottshambaugh commented Apr 30, 2023 •

edited

scottshambaugh commented Apr 30, 2023 •

edited

oscargus commented Apr 30, 2023

oscargus left a comment

scottshambaugh commented Apr 30, 2023 •

edited

tacaswell commented Aug 2, 2023

tacaswell commented Aug 2, 2023

QuLogic commented Aug 2, 2023

scottshambaugh commented Aug 3, 2023

scottshambaugh commented Sep 17, 2023

timhoffm commented Sep 18, 2023

ksunden commented Sep 18, 2023

Do not add padding to 3D axis limits when limits are manually set #25272

Do not add padding to 3D axis limits when limits are manually set #25272

Conversation

scottshambaugh commented Feb 21, 2023 • edited

PR Summary

PR Checklist

oscargus commented Feb 21, 2023

scottshambaugh commented Feb 21, 2023

scottshambaugh commented Feb 24, 2023

scottshambaugh commented Apr 30, 2023 • edited

scottshambaugh commented Apr 30, 2023 • edited

oscargus commented Apr 30, 2023

oscargus left a comment

Choose a reason for hiding this comment

scottshambaugh commented Apr 30, 2023 • edited

tacaswell commented Aug 2, 2023

tacaswell commented Aug 2, 2023

QuLogic commented Aug 2, 2023

scottshambaugh commented Aug 3, 2023

scottshambaugh commented Sep 17, 2023

timhoffm commented Sep 18, 2023

ksunden commented Sep 18, 2023

scottshambaugh commented Feb 21, 2023 •

edited

scottshambaugh commented Apr 30, 2023 •

edited

scottshambaugh commented Apr 30, 2023 •

edited

scottshambaugh commented Apr 30, 2023 •

edited