FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Issue #1888: added in the \dfrac macro for displaystyle fractions by watkinrt · Pull Request #8151 · matplotlib/matplotlib · GitHub

Repository navigation

Issue #1888: added in the \dfrac macro for displaystyle fractions - #8151

Merged
QuLogic merged 9 commits into
matplotlib:masterfrom
watkinrt:dfrac
Jul 26, 2017
Merged

QuLogic merged 9 commits into
matplotlib:masterfrom
watkinrt:dfrac

Conversation

Copy link
Copy Markdown
Contributor

This is a partial fix for issue #1888. The implementation of \dfrac{}{} addresses the issue of creating fractions with displaystyle formatting (which the author of #1888 would like to be able to do); however, it does not implement the more generic \displaystyle functionality as explicitly requested in #1888 (I've begun coding this up, but it is reasonably involved and has a number of pitfalls).

I added in an example script (dfrac_demo.py) to demonstrate the new functionality.

dstansby added this to the 2.1 (next point release) milestone Feb 26, 2017

Copy link
Copy Markdown
Member

There's a couple of PEP8 violations in the test - some trailing whitespace at the end of the lines, and no newline at the end of the file (see https://travis-ci.org/matplotlib/matplotlib/jobs/205418695#L1677)

@@ -0,0 +1,15 @@
#!/usr/bin/env python2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

remove the pinning to python2

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Will do. This is my first contribution, so I'm still working out some of the bugs. Thanks for the prompt suggestions.

Copy link
Copy Markdown
Member

Can you please add a test to https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_mathtext.py#L17 to cover this case?

Over all 👍 thanks for working on this!

Copy link
Copy Markdown
Member

To get the test images you will have to add the test, run the test suite, let if it fail on a missing image, then copy output into tests/baseline_images/test_mathtext and commit them (see http://matplotlib.org/devel/testing.html#writing-an-image-comparison-test)

You will also need to be sure to be testing against the correct version of freetype (see http://matplotlib.org/devel/contributing.html#retrieving-and-installing-the-latest-version-of-the-code)

#!/usr/bin/env python2
# -*- coding: utf-8 -*-
"""
Created on Sat Feb 25 17:50:37 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Remove this date; we have git log for that. Can you please modify the docstring to follow the sphinx-gallery format?

fig.text(0.5, 0.3, r'\dfrac: $\dfrac{a}{b}$',
horizontalalignment='center', verticalalignment='center')
fig.text(0.5, 0.7, r'\frac: $\frac{a}{b}$',
horizontalalignment='center', verticalalignment='center')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Needs plt.show at the end.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Man, you guys are fast and thorough. I'm working on all of the requested changes.

assert(len(toks)==1)
assert(len(toks[0])==6)
assert(len(toks) == 1)
assert(len(toks[0]) == 6)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍 for the whitespace fix, but also, you don't really need the parentheses.

Bug fix: I added a plt.show() to the end of the dfrac_demo
Tests: I added in a new test for the dfrac macro (along with the corresponding comparison files) within the test_mathtex test suite.
@@ -0,0 +1,28 @@
"""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This needs to be a raw string or all the backslashes need to be escaped (which probably won't look that great.)

num.shrink()
den.shrink()

# If style != displaystyle == 0, shrink the num and den

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think these warrant some constants instead of magic numbers.

…string with the backslashes escaped.

Feature modification: I added in a dictionary to the Parser class of mathtex to link math style names to their TeX numerical values. I used a dictionary instead of hardcoding specific math style (ex. DISPLAYSTYLE=0) as I will use the dictionary in my implementation of the math styles in the future.
tacaswell closed this Mar 2, 2017
tacaswell reopened this Mar 2, 2017

Copy link
Copy Markdown
Member

''power-cycled' to trigger travis.

QuLogic commented Mar 2, 2017

Copy link
Copy Markdown
Member

It needs to be a raw string or escaped backslashes, not both.

watkinrt commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Am I missing something? The reStructuredText documents make it sound like you need to make the text is a raw string to properly pass the backslashes out of python, but that you still need to escape the backslashs for the reStructuredText to be parsed correctly (http://docutils.sourceforge.net/docs/user/rst/quickref.html#escaping).

QuLogic commented Mar 25, 2017

Copy link
Copy Markdown
Member

At this point, we don't actually render any of that text through Sphinx. After #8247 is merged, then maybe it will be clearer what should be done.

… as matplotlib doesn't currently support this behavior in docstrings.

In this example, the differences between the \dfrac and \frac TeX macros are
illustrated; in particular, the difference between display style and text style
fractions when using Mathtex. aasdsads

anntzer Apr 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

mathtext
remove whatever aasdsads is

QuLogic commented Apr 30, 2017

Copy link
Copy Markdown
Member

Do you want to have a go at rebasing this? The sphinx-gallery stuff is merged now, so we should be able to see whether the escaping is going to be a problem.

watkinrt commented May 3, 2017 via email

Copy link
Copy Markdown
Contributor Author

…and updated the docstring to appropriately escape the backslashes.

Copy link
Copy Markdown
Member

It looks like some of the new required test images did not get added

_________________ test_mathtext_rendering[png-mathtext-cm-82] __________________
[gw1] linux -- Python 3.4.6 /home/travis/build/matplotlib/matplotlib/venv/bin/python
self = <matplotlib.testing.decorators._ImageComparisonBase object at 0x7fffe12af160>
baseline = 'mathtext_cm_82', extension = 'png'
    def copy_baseline(self, baseline, extension):
        baseline_path = os.path.join(self.baseline_dir, baseline)
        orig_expected_fname = baseline_path + '.' + extension
        if extension == 'eps' and not os.path.exists(orig_expected_fname):
            orig_expected_fname = baseline_path + '.pdf'
        expected_fname = make_test_filename(os.path.join(
            self.result_dir, os.path.basename(orig_expected_fname)), 'expected')
        if os.path.exists(orig_expected_fname):
            shutil.copyfile(orig_expected_fname, expected_fname)
        else:
            reason = ("Do not have baseline image {0} because this "
                      "file does not exist: {1}".format(expected_fname,
                                                        orig_expected_fname))
>           raise ImageComparisonFailure(reason)
E           matplotlib.testing.exceptions.ImageComparisonFailure: Do not have baseline image /home/travis/build/matplotlib/matplotlib/result_images/test_mathtext/mathtext_cm_82-expected.png because this file does not exist: /home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/baseline_images/test_mathtext/mathtext_cm_82.png
lib/matplotlib/testing/decorators.py:271: ImageComparisonFailure

I suspect the issue is missing optional dependencies on the local system where the test images were generated. @watkinrt I can try to generate them and push to this branch or you can sort out which deps you are missing (my guess is LaTeX?), which ever you would prefer.

watkinrt commented Jul 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@watkinrt I can try to generate them and push to this branch or you can sort out which deps you are missing (my guess is LaTeX?), which ever you would prefer.

I just realized I hadn't fetched the master branch from the main matplotlib repository in a while; I image that is why I was missing some of the new test images. I've updated my working copy and just pushed it. I haven't been able to successfully run the full test suite on this commit as something is messed up with the my development copy of matplotlib. If someone else has time, would you mind running the tests? I'm not sure when I'll be able to get around to fixing my development copy.

Sorry for all of my newbiew mistakes; this is the first large scale repository/team I've worked on.

QuLogic commented Jul 15, 2017

Copy link
Copy Markdown
Member

It's preferable to rebase instead of merging master. However, the tests are failing because it looks like you forgot to commit the image for the new test.

Copy link
Copy Markdown
Member

@watkinrt I'll try to generate those test images for you now and push to your branch.

With your permission I can do the rebase for you (and force-push to your branch).

Don't worry about small process mistakes, we were all new once!

Copy link
Copy Markdown
Member

I pushed a commit and then almost immediately force-pushed a fixed version of it (accidentally committed a temporary file). Hopefully no one fetched between the two pushes, but if you did, that is the source of any issue.

Copy link
Copy Markdown
Member

Looks like this is passing now and the test images look good!

Copy link
Copy Markdown
Contributor Author

Great! Thanks for closing all of this out. Hopefully that wraps everything up for this pull request.

Copy link
Copy Markdown
Member

@QuLogic I think this is ready to go, can you give this another review or dismiss your change request?

QuLogic merged commit c45678e into matplotlib:master Jul 26, 2017

Copy link
Copy Markdown
Member

@watkinrt Thanks and congratulations on what (I think) is your first Matplotlib contribution 🎉 !

Thanks for taking on one of the more arcane bits of the code base and bearing with us through a very long review process!

Hopefully we will hear from you again.

Copy link
Copy Markdown
Contributor

Wow, this is great, @watkinrt ! I had no idea you had finished this almost 2 years ago!

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

6 participants


Back | FazBrowse Home | New Git URL