Issue 2899 #2923

sfroid · 2014-03-24T20:00:54Z

Adding wedgeprops and textprops arguments to pie, which are forwarded to the wedge and text objects respectively. This allows the user to control various properties of the pie directly (eg linewidth of the border of wedges).
Added image test to check linewidth of pies (for cases linewidth 0 and 2).

…edge and text elements separately

… to pie. needs feedback on generated pyplot

sfroid · 2014-03-24T21:01:53Z

@tacaswell @mdboom
Need some pointers on how to resolve failing tests

The autogenerated pyplot.py file has the prefix "u" appended to all string type function arguments. I can manually edit these to remove the prefix "u", but that would defeat the purpose of an autogenerated file. How do I fix this?
I am getting an asserting error, which seems to be unrelated to the changes I made...

======================================================================
FAIL: Github issue #1256 identified a bug in Line.draw method
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.6/lib/python2.6/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/virtualenv/python2.6/lib/python2.6/site-packages/matplotlib-1.4.x-py2.6-linux-x86_64.egg/matplotlib/testing/decorators.py", line 110, in wrapped_function
    func(*args, **kwargs)
  File "/home/travis/virtualenv/python2.6/lib/python2.6/site-packages/matplotlib-1.4.x-py2.6-linux-x86_64.egg/matplotlib/tests/test_lines.py", line 58, in test_invisible_Line_rendering
    assert_true(slowdown_factor < slowdown_threshold)
AssertionError

Any idea what could be going wrong?

It seems to me as if this test could fail depending on how long it takes for the rendering function. It probably is not something caused by my changes.

tacaswell · 2014-03-25T12:35:47Z

I would ignore those test failures, that one pops up from time to time (free cpu cycles are not guaranteed cpu cycles).

The problem with the unicode strings come from the from __future__ import unicode_literals at the top of every file which is part of the python2/python3 compatibility layer. boilerplate.py does some introspection and then generates pyplot.py and (correctly) writes out all of the strings as unicode. However, this ends up being a problem because in python 3 the default in unicode strings and in 3.2 it will raise an error if you give it a u'blah' string. I think last time this came up I applied sed to the output....

sfroid · 2014-03-26T00:23:14Z

@tacaswell
This is ready to be merged... its now on your plate, whenever you have the time.

I am now starting to look at the needs_patch issues.

tacaswell · 2014-03-26T00:36:04Z

This looks great! Thanks for sorting out the issue with boilerplate.

I am happy with these changes, but I would like @mdboom or @efiring to take a look at the changes to boiler plate.

I also don't recall of the top of my head if we want to keep all of the images for all of the tests (png might be enough).

efiring · 2014-03-26T05:37:40Z

doc/devel/coding_guide.rst

 a plotting method from pyplot, edit the appropriate list in `boilerplate.py`
 and then run the script which will update the content in
 `lib/matplotlib/pyplot.py`. Both the changes in `boilerplate.py` and
 `lib/matplotlib/pyplot.py` should be checked into the repository.
+
+Note: boilerplate.py looks for changes in the installed version of matplotlib


Five years ago, in e91c5a1, I commented out the line that was making boilerplate look first in the local tree, without removing the comment that went with that line. I don't know why I did that. If the present behavior is what we want, then the first two lines in the excerpt from boilerplate, below, should be deleted. I suspect the present behavior makes sense because otherwise python code would be found in the source tree but compiled extensions would be from the installed version, so they could be out of sync.

# import the local copy of matplotlib, not the installed one #sys.path.insert(0, './lib') from matplotlib.axes import Axes

@efiring
Updated boilerplate.py with a better comment.

efiring · 2014-03-26T20:07:25Z

Travis PEP8 failures are unrelated to this PR. Were modules such as animation and patches previously exempt from PEP8 testing, but recently included? Or are more PEP8 tests now being used as criteria for rejection? Or did a merge accidentally revert the modules to a much earlier state? I don't think it is the last of these, fortunately.

tacaswell · 2014-03-26T23:39:17Z

Oddly, one of the builds (2.7) seems to have passed even though the others failed for pep8 reasons.....

sfroid · 2014-03-27T01:33:17Z

@efiring @tacaswell
The travis failure was indeed unexpected as my changes passed the Travis tests yesterday (after the second last commit). My last looks completely harmless/unrelated to any kind of tests.

I have no idea of any modules being PEP8 exempt. I jumped on the matplotlib dev bandwagon last week and this is literally my first PR to this repo. Also, I don't see any merge related changes in the diff for this PR.

tacaswell · 2014-03-27T01:54:24Z

@sfroid See #2930 . The issue is that pep8 bumped it's version today which seems to have introduced a bunch of new tests. I suspect the one that passed got a cached copy of the old pep8 install.

sfroid · 2014-03-27T04:54:14Z

@tacaswell @efiring
What is the suggested solution here? Fix all the pep8 reported issues (334 in all, but each of them quite simple)?
Or do we have another way to handle this?

tacaswell · 2014-03-27T12:07:10Z

The other PR squelches the tests that we did not run before.

Please don't start fixing pep8 issues in this PR, if we are going to do that we should do it in a dedicated PR. Style conflicts are the worst to merge.

Issue 2899

sfroid · 2014-03-27T21:25:44Z

Yayy!! My first contribution to matplotlib.
Thank you @tacaswell and @efiring for the help.

efiring · 2014-03-27T21:52:55Z

Thank you @sfroid for the contribution.

tacaswell · 2014-03-28T12:07:21Z

Thank you @sfroid !

sfroid added 5 commits March 22, 2014 03:18

adding the linewidth keyword argument to pie

3b767f6

matplotlib#2899 Adding kwargs to pie which can handle args for both w…

3ceade6

…edge and text elements separately

matplotlib#2899 almost final changes for adding wedge and text kwargs…

bd55ab2

… to pie. needs feedback on generated pyplot

cleaning up code. Added tests and test images

7cb6066

removing empty line for pep8 test

06fd264

fixing pyplot.py generating boilerplate code to avoid unicode strings

d618546

tacaswell added the needs_review label Mar 26, 2014

tacaswell added this to the v1.4.0 milestone Mar 26, 2014

efiring reviewed Mar 26, 2014
View reviewed changes

updating outdated comment in boilerplate.py

3523760

sfroid mentioned this pull request Mar 26, 2014

add ccw pie test #2897

Closed

efiring added a commit that referenced this pull request Mar 27, 2014

Merge pull request #2923 from sfroid/issue_2899

be33461

Issue 2899

efiring merged commit be33461 into matplotlib:master Mar 27, 2014

sfroid deleted the issue_2899 branch March 27, 2014 21:25

This was referenced Apr 23, 2014

BUG : turned clipping off on pie chart components #2952

Merged

adding linewidth argument to pie #2899

Closed

tacaswell mentioned this pull request Jun 11, 2014

axes.pie should support patch kwargs #3134

Open

QuLogic removed the status: needs review label Nov 24, 2016

story645 mentioned this pull request Nov 15, 2022

[ENH] hatch keyword for pie + some pie documentation #24470

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 2899 #2923

Issue 2899 #2923

sfroid commented Mar 24, 2014

sfroid commented Mar 24, 2014

tacaswell commented Mar 25, 2014

sfroid commented Mar 26, 2014

tacaswell commented Mar 26, 2014

efiring Mar 26, 2014

sfroid Mar 26, 2014

efiring commented Mar 26, 2014

tacaswell commented Mar 26, 2014

sfroid commented Mar 27, 2014

tacaswell commented Mar 27, 2014

sfroid commented Mar 27, 2014

tacaswell commented Mar 27, 2014

sfroid commented Mar 27, 2014

efiring commented Mar 27, 2014

tacaswell commented Mar 28, 2014

Issue 2899 #2923

Issue 2899 #2923

Conversation

sfroid commented Mar 24, 2014

sfroid commented Mar 24, 2014

tacaswell commented Mar 25, 2014

sfroid commented Mar 26, 2014

tacaswell commented Mar 26, 2014

efiring Mar 26, 2014

Choose a reason for hiding this comment

sfroid Mar 26, 2014

Choose a reason for hiding this comment

efiring commented Mar 26, 2014

tacaswell commented Mar 26, 2014

sfroid commented Mar 27, 2014

tacaswell commented Mar 27, 2014

sfroid commented Mar 27, 2014

tacaswell commented Mar 27, 2014

sfroid commented Mar 27, 2014

efiring commented Mar 27, 2014

tacaswell commented Mar 28, 2014