Skip to content

DOC: fix layout pad #25031

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jan 19, 2023

PR Summary

The layout pads in the constrained layout docs were incorrectly stated as being in figure-normalized units, whereas they are actually in inches. This was correct in most places, except where it mattered the most.

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).
  • New plotting related features are documented with examples.

Release Notes

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

@jklymak jklymak added this to the v3.7.0 milestone Jan 19, 2023
@@ -76,7 +76,7 @@ def do_constrained_layout(fig, h_pad, w_pad,
Renderer to use.

h_pad, w_pad : float
Padding around the axes elements in figure-normalized units.
Padding around the axes elements in inches.
Copy link
Member

Choose a reason for hiding this comment

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

I think this one was correct, because the layout engine does the normalisation before calling this function

# pads are relative to the current state of the figure...
w_pad = self._params['w_pad'] / width
h_pad = self._params['h_pad'] / height
return do_constrained_layout(fig, w_pad=w_pad, h_pad=h_pad,
wspace=self._params['wspace'],
hspace=self._params['hspace'],
rect=self._params['rect'],
compress=self._compress)

Copy link
Member Author

Choose a reason for hiding this comment

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

....oh, thats why its so confused. OK, I'll look into it more carefully.

@@ -203,7 +203,7 @@ def __init__(self, *, h_pad=None, w_pad=None,
Parameters
----------
h_pad, w_pad : float
Padding around the axes elements in figure-normalized units.
Padding around the axes elements in inches.
Copy link
Member

@rcomer rcomer Jan 19, 2023

Choose a reason for hiding this comment

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

We did this one at #24981

Edit: sorry I just noticed you are targetting this for 3.7. So this one stands 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't being that subtle - I didn't catch that it was done in #24981.

@rcomer
Copy link
Member

rcomer commented Jan 19, 2023

There is also the set method on the layout engine, which has the same problem.

@jklymak jklymak marked this pull request as draft January 19, 2023 18:46
@jklymak
Copy link
Member Author

jklymak commented Feb 2, 2023

@rcomer So I'm confused what needs to be done here, if anything. Should I just close as already fixed?

@rcomer
Copy link
Member

rcomer commented Feb 2, 2023

I think it’s already fixed in main. It could be argued that it’s worth also fixing in 3.7.

@jklymak
Copy link
Member Author

jklymak commented Feb 2, 2023

I think this has been wrong long enough to wait for 3.8 ;-)

@jklymak jklymak closed this Feb 2, 2023
@jklymak jklymak deleted the doc-fix-layout-pad branch February 2, 2023 22:26
@tacaswell tacaswell removed this from the v3.7.0 milestone Feb 2, 2023
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