Skip to content
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

Issue 2899 #2923

Merged
merged 7 commits into from Mar 27, 2014
Merged

Issue 2899 #2923

merged 7 commits into from Mar 27, 2014

Conversation

sfroid
Copy link
Contributor

@sfroid sfroid commented Mar 24, 2014

#2899

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).

@sfroid
Copy link
Contributor Author

sfroid commented Mar 24, 2014

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

  1. 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?
  2. 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
Copy link
Member

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
Copy link
Contributor Author

sfroid commented Mar 26, 2014

@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 tacaswell added this to the v1.4.0 milestone Mar 26, 2014
@tacaswell
Copy link
Member

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).

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@efiring
Updated boilerplate.py with a better comment.

@sfroid sfroid mentioned this pull request Mar 26, 2014
@efiring
Copy link
Member

efiring commented Mar 26, 2014

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
Copy link
Member

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

@sfroid
Copy link
Contributor Author

sfroid commented Mar 27, 2014

@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
Copy link
Member

@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
Copy link
Contributor Author

sfroid commented Mar 27, 2014

@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
Copy link
Member

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.

efiring added a commit that referenced this pull request Mar 27, 2014
@efiring efiring merged commit be33461 into matplotlib:master Mar 27, 2014
@sfroid
Copy link
Contributor Author

sfroid commented Mar 27, 2014

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

@sfroid sfroid deleted the issue_2899 branch March 27, 2014 21:25
@efiring
Copy link
Member

efiring commented Mar 27, 2014

Thank you @sfroid for the contribution.

@tacaswell
Copy link
Member

Thank you @sfroid !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants