Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

FIX: add set_box_aspect, improve tight bounding box for Axes3D + fix bbox_inches support with fixed box_aspect#17515

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

Merged
QuLogic merged 12 commits intomatplotlib:masterfromtacaswell:fix_3D_tight_aspect
Jun 6, 2020

Conversation

tacaswell
Copy link
Member

PR Summary

To make bbox_inches='tight' work correctly, we cache the positions of
all of the axes, change their aspect to 'auto', adjust the transforms
on the figure, render the output, and then restore the previous
setting of aspect to each of the figures. This prevent the Axes from
trying to adjust their size on draw. This matters because for the
render that is emitted the figure transforms are out of sync with the
figure size.

As part of cleaning fixing the rendering of Axes3D in#8896 /#16472
we started to use apply_aspect to re-size the area the Axes3D takes up
to maintain a fixed ratio between the axes when the aspect is "auto".
However this conflicts with the expectation in tight_bbox.adjust_bbox
as it assumes setting the aspect to "auto" will prevent any axes
resizing.

This commit addresses this by:

  • exiting the Axes3D.apply_aspect early if aspect is auto
  • adding a new aspect mode 'auto_pb' which is the default
    for Axes3D which maintains the current master branch behavior
    of maintaining a fixed ratio between the sizes of the x, y z axis
    independent of the data limits.
  • re implement several functions on Axes3D to make sure they handle
    sharez correctly.

closes#16463.

Starting from@ImportanceOfBeingErnest example

importmatplotlib.pyplotaspltfrommpl_toolkits.mplot3dimportAxes3Dplt.rcParams["axes.facecolor"]="yellow"fig,ax=plt.subplots(dpi=72,figsize=(6,3),subplot_kw={"projection" :"3d"})print(ax.get_aspect())plt.savefig("/tmp/test1.png",facecolor="cyan")plt.savefig("/tmp/test2.png",facecolor="cyan",bbox_inches="tight")plt.show()

test1
test2

so better! but we still need at least one more commit fix the tight box computation on the 3D axes.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswelltacaswell added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelMay 27, 2020
@tacaswelltacaswell added this to thev3.3.0 milestoneMay 27, 2020
@tacaswell
Copy link
MemberAuthor

attn@eric-wieser .

attn@jklymak I tried the naive thing patch to Axes3D, but is seems to not be sufficient, what am I missing about how we compute bounding boxes?

@jklymak
Copy link
Member

I dunno, but I guess it has never worked?

defget_tightbbox(self,renderer):
# Currently returns None so that Axis.get_tightbbox
# doesn't return junk info.
returnNone

@tacaswell
Copy link
MemberAuthor

Ah, that makes sense, I forgot we replaced the axis classes too.

@tacaswelltacaswell marked this pull request as ready for reviewMay 27, 2020 21:38
@tacaswelltacaswell mentioned this pull requestMay 27, 2020
@tacaswell
Copy link
MemberAuthor

The appevyor fail is an upload failure:

found artifact 'dist\matplotlib-3.2.1+5180.g75eedddb-cp37-cp37m-win_amd64.whl' matching 'dist\*' path5144No artifacts found matching 'result_images\*' path5145Uploading artifacts...5146Error uploading artifact the storage: The remote name could not be resolved: 'appveyorcidatav2.blob.core.windows.net'

Related, we should definitely document where those wheels are getting published!

@tacaswell
Copy link
MemberAuthor

womp_womp

This is what the new test looks like (with text) without390a57a

@tacaswelltacaswell changed the titleENH/FIX: make "auto" aspect with Axes3D match Axes betterFIX: add set_box_aspect, improve tight bounding box for Axes3D + fix bbox_inches support with fixeh box_aspectMay 30, 2020
@@ -99,7 +102,9 @@ def __init__(
self._shared_z_axes.join(self, sharez)
self._adjustable = 'datalim'

super().__init__(fig, rect, frameon=True, *args, **kwargs)
super().__init__(
fig, rect, frameon=True, box_aspect=box_aspect, *args, **kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Huh, shouldn't these keyword arguments be after*args? I guess it must still work though.

@QuLogic
Copy link
Member

Would fix#17172 as well?

@tacaswelltacaswell changed the titleFIX: add set_box_aspect, improve tight bounding box for Axes3D + fix bbox_inches support with fixeh box_aspectFIX: add set_box_aspect, improve tight bounding box for Axes3D + fix bbox_inches support with fixed box_aspectJun 3, 2020
@tacaswell
Copy link
MemberAuthor

Yes, this will fix#17172 as well.

Changes the physical dimensions of the Axes, such that the ratio
of the size of the axis in physical units is x:y:z

The input will be normalized to a unit vector.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see normalization to a unit vector in the code below. And, is any normalization of the 3 numbers specifyingratios relevant to the user?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The magnitude is controlled by zoom. Maybe phrase this as "Only the ratio matters, the magnitude is discarded"?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe replaceboth sentences with,

"Sets the ratios of the axis lengths in physical units as x:y:z."

And, earlier, just give the default, (4, 4, 3). If you leave it at that, then giving the zoom with its default and minimal explanation will complete the basics. To go farther, you probably need an example, e.g., what might the call look like if you are making a plot of lake bathymetry and you want 1 m in the vertical to match 100 m in the horizontal. I'm assuming that the sort of thing this is for, but I don't fully understand.

Are these ratios showing the length ratios you would see for a cube viewed along each of 3 axes? So if you have (3, 4, 5), then looking down from the top, the y-axis/x-axis length ratio of the rectangular projection would be 4/3, and looking along the y-axis, the z-axis/x-axis ratio would be 5/3, etc.?

tacaswelland others added5 commitsJune 3, 2020 22:17
closesmatplotlib#17172Instead of adding a new method, reuse `box_aspect` expecting a 3vector.The very long float is to keep the tests passing and preserve thebehavior currently on master branch.
The way that bbox_inches='tight' is implemented we need to ensure thatwe do not try to adjust the aspect during the draw (because we havetemporarily de-coupled the reported figure size from the transformswhich results in the being distorted).  Previously we did not have away to fix the aspect ratio in screen space of the Axes (only theaspect ratio in dataspace) however in 3.3 we gained this ability forboth Axes (matplotlib#14917) and Axes3D (matplotlib#8896 /matplotlib#16472).Rather than add an aspect value to `set_aspect` to handle this case,in the tight_bbox code we monkey-patch the `apply_aspect` method witha no-op function and then restore it when we are done.  Previously wewould set the aspect to "auto" and restore it in the same places.closesmatplotlib#16463.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@tacaswell
Copy link
MemberAuthor

@efiring The docstring clearer?

@efiring
Copy link
Member

Summary:

  • Copy the good paragraph from whatsnew to the set_aspect or set_box_aspect docstring.

@tacaswell Thank you, it's getting better. The remaining overall doc problem is that the only place where there is a useful hint as to how a user can get a desired result is the very helpful paragraph in the whatsnew. I suggest copying this into the docstring, maybe as a note, for either set_aspect or set_box_aspect, or both. As it is now, without that, the set_aspect docstring basically says it doesn't do anything (only option is auto, which is not explained), and refers to the set_box_aspect docstring for more info. Now, the set_box_aspect docstring is almost comprehensible but it's pretty hard to be sure one is correctly visualizing what it is doing, especially in relation to the do-nothing set_aspect. So the poor user gets to the bottom of set_box_aspect docstring, hoping for more hints, and sees only a request to refer to the set_aspect docstring for more info--which isn't there. (It's a docstring reference cycle--call in the garbage collector!)

@tacaswell
Copy link
MemberAuthor

I started with the paragraph in the whats new in both docstrings and then edited to make it flow better.

Copy link
Member

@efiringefiring left a comment

Choose a reason for hiding this comment

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

Good!

ax.set_axes_locator(loc)
# delete our no-op function which un-hides the
# original method
del ax.apply_aspect
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to get confirmation that this covers all the cases that#17561 is supposed to now handle, e.g., overridden methods, etc..

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I think it now handles everything. If it does not exist, things are super broken so we only have to handleapply_aspect being in the instance dict or not.

We don't have anything in the test suite that exercises the "in instance dict" code path in the test suite.

tacaswelland others added2 commitsJune 5, 2020 15:00
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jklymakjklymakjklymak left review comments

@timhoffmtimhoffmtimhoffm left review comments

@efiringefiringefiring approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Jupyter "inline" backend seems to misinterpret "figsize" with Axes3D
6 participants
@tacaswell@jklymak@QuLogic@efiring@timhoffm@eric-wieser
[8]ページ先頭
©2009-2025 Movatter.jp