X Tutup
Skip to content

Fixed Error: local variable 'xdata' referenced before assignment" in legend_handler.py#8478

Merged
phobson merged 1 commit intomatplotlib:masterfrom
kalagau:TeamDoItTomorrowfix-6921
Apr 18, 2017
Merged

Fixed Error: local variable 'xdata' referenced before assignment" in legend_handler.py#8478
phobson merged 1 commit intomatplotlib:masterfrom
kalagau:TeamDoItTomorrowfix-6921

Conversation

@kalagau
Copy link
Copy Markdown
Contributor

@kalagau kalagau commented Apr 13, 2017

This is the PR for #6921

story645
story645 reviewed Apr 13, 2017
View reviewed changes
lib/matplotlib/legend_handler.py Outdated
numpoints = self.get_numpoints(legend)

if numpoints > 1:
if ((numpoints < 1) or not (isinstance(numpoints, int) or numpoints.is_integer())):
Copy link
Copy Markdown
Member

@story645 story645 Apr 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What's the difference between these two?
isinstance(numpoints, int) or numpoints.is_integer()

Copy link
Copy Markdown
Contributor Author

@kalagau kalagau Apr 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

ahh they do the same thing. Which style do you prefer?
Edit: Decided to stick to numpoints.is_integer()

QuLogic
QuLogic reviewed Apr 13, 2017
View reviewed changes
lib/matplotlib/legend_handler.py Outdated
numpoints = self.get_numpoints(legend)

if numpoints > 1:
if ((numpoints < 1) or not (numpoints.is_integer())):
Copy link
Copy Markdown
Member

@QuLogic QuLogic Apr 13, 2017

Choose a reason for hiding this comment

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

You should check for integralness first, I think. Also, most of these parentheses are unnecessary.

lib/matplotlib/legend_handler.py Outdated
if ((numpoints < 1) or not (numpoints.is_integer())):
raise ValueError("numpoints must be a whole number and greater" +
" than or equal to 1; it was", numpoints)
elif (numpoints == 1):
Copy link
Copy Markdown
Member

@QuLogic QuLogic Apr 13, 2017

Choose a reason for hiding this comment

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

Unnecessary parentheses.

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Apr 14, 2017

I would just have a check if > 1: ... else: ... (basically just remove the ==1)
Non-integer values will trigger a warning in linspace ("DeprecationWarning: object of type <class 'float'> cannot be safely interpreted as an integer.") which should be enough.
Otherwise, adding typechecks everywhere is just a never ending task.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Apr 14, 2017
@anntzer anntzer mentioned this pull request Apr 14, 2017
Closed
@kalagau
Copy link
Copy Markdown
Contributor Author

kalagau commented Apr 14, 2017

@anntzer I was just fixing it up.

@tacaswell
Copy link
Copy Markdown
Member

tacaswell commented Apr 16, 2017

@kalagau This looks like you have mixed tabs and spaces! Can you replace them all with 4 spaces (most editors will have a tool for this (ex untabify in emacs) or just find and replace)?

tacaswell
tacaswell approved these changes Apr 17, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Adding a test

def test_silly_numpoints():
    fig, ax = plt.subplots()
    ax.plot(range(5), label='test')
    ax.legend(numpoints=0.5)

would not be bad, but I do not think is essential.

@kalagau
Copy link
Copy Markdown
Contributor Author

kalagau commented Apr 17, 2017

Ill add it in later today

@kalagau
Copy link
Copy Markdown
Contributor Author

kalagau commented Apr 17, 2017

I think this should be good now! all tests are passing!

@phobson
Copy link
Copy Markdown
Member

phobson commented Apr 17, 2017

@kalagau Are you comfortable squashing this down to 1 commit?

@kalagau
fix issue 6921 and add tests
f3b82c1 
Fix Error: local variable 'xdata' referenced before assignment" in legend_handler.py
@kalagau kalagau force-pushed the TeamDoItTomorrowfix-6921 branch from 477a129 to f3b82c1 Compare April 17, 2017 19:43
@kalagau
Copy link
Copy Markdown
Contributor Author

kalagau commented Apr 17, 2017

Sure @phobson, just did it.

@phobson phobson merged commit 481e860 into matplotlib:master Apr 18, 2017
@phobson
Copy link
Copy Markdown
Member

phobson commented Apr 18, 2017

thanks @kalagau !

@phobson phobson mentioned this pull request Apr 18, 2017
Closed
@anntzer anntzer mentioned this pull request Apr 18, 2017
Closed
@kalagau kalagau deleted the TeamDoItTomorrowfix-6921 branch April 19, 2017 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic left review comments

@story645 story645 story645 left review comments

@tacaswell tacaswell tacaswell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.1

Development

Successfully merging this pull request may close these issues.

6 participants

@kalagau @anntzer @tacaswell @phobson @QuLogic @story645
X Tutup