Skip to content

Doc: beautify usetex demo example #11462

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

Conversation

ImportanceOfBeingErnest
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Jun 20, 2018

PR Summary

Beautify usetex demo. This demo was too crowded and contained unnecessary commands. This PR simplifies and beautifies it in the hope to lower the risk of eye cancer.

In total I'm not convinced this demo is entirely necessary, but the way I changed it now, it at least shows some interesting things like the combination of math mode and text mode in one text, or the use of eqnarray environements.

old

image

new
image

PR Checklist

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

@jklymak
Copy link
Member

jklymak commented Jun 20, 2018

This is better - any chance the \gamma and \omega could be separated vertically more?

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the doc_beautify-usetex-example branch from b73f96b to e7255c7 Compare June 20, 2018 15:14
@ImportanceOfBeingErnest
Copy link
Member Author

Sure. Updated.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

A big improvement, both in text and in code. Found two minor code issues.

'k', linewidth=2)
plt.text(-0.06, height - 0.06, r'$\delta$', {'color': 'k', 'fontsize': 24})
plt.annotate("",
xy=(-delta / 2., 0.1), xycoords='data',
Copy link
Member

Choose a reason for hiding this comment

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

This and the following lines could use one more space.

"""

import matplotlib
matplotlib.rc('text', usetex=True)
Copy link
Member

Choose a reason for hiding this comment

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

Not your fault, but I would do the rc change after all the imports.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Wait for merge for minor comments...

@jklymak jklymak added this to the v2.2-doc milestone Jun 20, 2018
@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the doc_beautify-usetex-example branch from e7255c7 to bb46650 Compare June 20, 2018 20:54
@ImportanceOfBeingErnest
Copy link
Member Author

Updated and passed the tests.

@timhoffm timhoffm merged commit c1f0072 into matplotlib:master Jun 20, 2018
@ImportanceOfBeingErnest ImportanceOfBeingErnest deleted the doc_beautify-usetex-example branch June 20, 2018 22:43
@QuLogic
Copy link
Member

QuLogic commented Jun 20, 2018

This example doesn't pass PEP8 (see backport's build failure), but I don't know why master build is not seeing it.

@ImportanceOfBeingErnest
Copy link
Member Author

This is annoying. It happened already previously. It's either only not checking it for my PRs or it's not checking it for some parts. What would you suggest that I do about it, given that this is already merged into master?

@QuLogic
Copy link
Member

QuLogic commented Jun 22, 2018

The PEP8 checks should be re-enabled on master by #11477.

jklymak added a commit that referenced this pull request Jul 3, 2018
…n-v2.2.2-doc

Backport PR #11462 on branch v2.2.2-doc
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.

4 participants