Skip to content

DOC: fix CL tutorial to give same output from saved file and example #12394

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

Merged

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Oct 3, 2018

PR Summary

Closes #12391.

When CI passes I'll post the result here for review....

As it'll post in the docs

cl

As it looks in saved file

outname

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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

fig.savefig('outname.png', bbox_inches='tight')

#############################################
# A better way to get around this awkwardness is to simply
# use a legend for the figure:
# use the legend method provided by the figure class:
Copy link
Member

Choose a reason for hiding this comment

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

simply use `.Figure.legend`:

@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.0.0-doc milestone Oct 4, 2018
@ImportanceOfBeingErnest
Copy link
Member

Yes this works now as expected.
Did you consider point 3. from the list in #12391? (If you think this doesn't apply, that's fine.)

Maybe the text should clearly state here that in order to see the effect one would need to compare the shown (add plt.show() at the end?) with the saved figure? Maybe the line wanttoprint = False can be appended with a comment # set to True and compare saved figure or similar?

@jklymak
Copy link
Member Author

jklymak commented Oct 4, 2018

Can we include images in the tutorials? I guess I could just include the png.

WRT item 3 I was t sure it was necessary but happy to do it if you feel it helps

@ImportanceOfBeingErnest
Copy link
Member

I think some of the confusion comes from looking at the produced images, so if at least the code has the savefig command in it, it would point more towards where to look for the feature.

I think you can use rst inside the examples, so .. image:: some.png might just work.

@ImportanceOfBeingErnest
Copy link
Member

The manually added figures are a bit... HUGE.

image

But appart this looks good. With both figures shown like this I think the whole section becomes really clear.

@jklymak jklymak force-pushed the doc-fix-cl-tutorial branch from fc9967d to e84c56c Compare October 4, 2018 15:17
@jklymak
Copy link
Member Author

jklymak commented Oct 4, 2018

@ImportanceOfBeingErnest
Copy link
Member

ImportanceOfBeingErnest commented Oct 5, 2018

What's the reason for the difference? Shouldn't it save with the same dpi as it shows?

Ah, you changed the width... Why not produce the figure with savefig(..., dpi=100) locally?

@jklymak
Copy link
Member Author

jklymak commented Oct 5, 2018

I had to hard-set the width to 300 px in the rst. We are only trying to show what the layout looks like, not what the exact sizing of the png is.

@ImportanceOfBeingErnest
Copy link
Member

image

Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

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

This makes the Legend section of the constrained layout guide much clearer and it's well suited to prevent misunderstandings as in #12377.

# The saved file looks like:
#
# .. image:: /_static/constrained_layout/CL02.png
# :align: center

Choose a reason for hiding this comment

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

The center alignment seems to be ignored for some reason by the css. I would not consider this crucial at this point. One can revisit the cause of this at a different stage.

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 well I tried. As I'm sure you can tell screwing around w/ rst embedded in python comments isn't my favourite pastime.

Choose a reason for hiding this comment

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

There is nothing wrong with the rst command itself. It's either sphinx having changed something in their newest version or us screwing up the CSS.

I think adding something like

img.align-center {    
    display: block;
    margin-left: auto;
    margin-right: auto;}

in the css file will center the image correctly.

It's something to keep in mind, but somehow unrelated to this PR anyways.

@jklymak jklymak force-pushed the doc-fix-cl-tutorial branch from b38f71a to d71b44b Compare October 5, 2018 14:12
@ImportanceOfBeingErnest ImportanceOfBeingErnest merged commit 8ba6651 into matplotlib:master Oct 5, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 5, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 5, 2018
@jklymak jklymak modified the milestones: v3.0.0-doc, v3.1 Oct 5, 2018
@jklymak jklymak deleted the doc-fix-cl-tutorial branch October 5, 2018 18:14
@jklymak
Copy link
Member Author

jklymak commented Oct 5, 2018

Thanks for the help @ImportanceOfBeingErnest

jklymak added a commit that referenced this pull request Oct 5, 2018
…394-on-v3.0.x

Backport PR #12394 on branch v3.0.x (DOC: fix CL tutorial to give same output from saved file and example)
jklymak added a commit that referenced this pull request Oct 5, 2018
…394-on-v3.0.0-doc

Backport PR #12394 on branch v3.0.0-doc (DOC: fix CL tutorial to give same output from saved file and example)
@QuLogic QuLogic modified the milestones: v3.1, v3.0.0-doc Oct 6, 2018
@ImportanceOfBeingErnest
Copy link
Member

It seems one drawback of this is that now every time you build the docs it'll create two png images in the tutorials folder, which will then end up in any commit (if one doesn't pay attention).

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.

Constrained Layout tutorial needs some cleanup....
4 participants