[MRG+1] Add short-circuit return to matplotlib.artist.setp if input is length 0 #7801

Merged
merged 2 commits into from Jan 13, 2017

Projects

None yet

6 participants

@samsontmr
Contributor
samsontmr commented Jan 11, 2017 edited

Fixes #7784

Before this patch, an IndexError is raised in matplotlib.artist.setp if argument is an empty list,

import matplotlib
matplotlib.artist.setp([])
@samsontmr samsontmr changed the title from Add short-circuit return if input is length 0 to Add short-circuit return to `matplotlib.artist.setp` if input is length 0 Jan 11, 2017
@samsontmr samsontmr changed the title from Add short-circuit return to `matplotlib.artist.setp` if input is length 0 to Add short-circuit return to matplotlib.artist.setp if input is length 0 Jan 11, 2017
lib/matplotlib/artist.py
@@ -1421,6 +1421,10 @@ def setp(obj, *args, **kwargs):
>>> setp(lines, 'linewidth', 2, 'color', 'r') # MATLAB style
>>> setp(lines, linewidth=2, color='r') # python style
"""
+
+ if ((isinstance(obj, np.ndarray) and not obj.size) or
@anntzer
anntzer Jan 11, 2017 Contributor

Just check if not objs: instead (objs is a list defined immediately below).

@samsontmr
samsontmr Jan 11, 2017 Contributor

Wouldn't if not objs return false if objs is a list of empty lists? Or do you mean to check for if not obj?

@tacaswell
tacaswell Jan 11, 2017 Member

Agree using if not objs: would be better. I would also move this down to L1453 (right above the line with objs[0]) after we have normalized the input to a list.

@samsontmr
samsontmr Jan 11, 2017 Contributor

Oh I see! The if else statements guarantee a list, right?

@samsontmr samsontmr Add short-circuit return if input is length 0
91df80b
@samsontmr
Contributor

Commits squashed

@tacaswell
Member

Looks good! Could you also add a test to test_artist.py that exercises this code path?

@phobson
Member
phobson commented Jan 11, 2017

Apologies if I'm missing something obvious here, but couldn't this be at the very top of the function? e.g.,

    if not objs:
        return
    elif not cbook.iterable(obj):
        objs = [obj]
    else:
        objs = list(cbook.flatten(obj))
@tacaswell
Member

@phobson users will find pathological cases 😉

In [62]: bool([[]])
Out[62]: 
True
@phobson
Member
phobson commented Jan 11, 2017

@tacaswell wow. great catch with that.

@samsontmr
Contributor
samsontmr commented Jan 12, 2017 edited

@phobson objs doesn't exist at the top either, only obj ;)
@tacaswell will do!

@samsontmr samsontmr Add tests for `[]` and `[[]]` in `test_artist.py`
193da2e
@codecov-io

Current coverage is 62.10% (diff: 100%)

Merging #7801 into master will decrease coverage by 0.01%

@@             master      #7801   diff @@
==========================================
  Files           174        174          
  Lines         56028      56052    +24   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34803      34809     +6   
- Misses        21225      21243    +18   
  Partials          0          0          

Powered by Codecov. Last update 9b1ece0...193da2e

@samsontmr
Contributor

@tacaswell Tests added

@NelleV NelleV changed the title from Add short-circuit return to matplotlib.artist.setp if input is length 0 to [MRG+1] Add short-circuit return to matplotlib.artist.setp if input is length 0 Jan 13, 2017
@NelleV
NelleV approved these changes Jan 13, 2017 View changes

LGTM

@NelleV
Contributor
NelleV commented Jan 13, 2017

I've edited your comment to explain what it fixes. It saves us reviewers a lot of time not to have to identify what the original problem was.

@tacaswell tacaswell merged commit 9edcb03 into matplotlib:master Jan 13, 2017

5 checks passed

codecov/patch 100% of diff hit (target 62.11%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +37.88% compared to 9b1ece0
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 62.101%
Details
@tacaswell
Member

This should be backported after 2.0

@samsontmr Thank!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment