better input validation on `fill_between` #7817

samsontmr · 2017-01-13T05:59:55Z

NelleV

Overall, it looks good, except for the failing tests.
I am not a big fan of the name input in the code, but haven't come up with something better (yet).

Once the tests are fixed, I think this can be merged.

NelleV · 2017-01-13T23:47:11Z

lib/matplotlib/tests/test_axes.py

@@ -725,6 +725,42 @@ def test_polycollection_joinstyle():
    ax.set_ybound(0, 3)


+@raises(ValueError)


I have no clue why this doesn't fail for all the builds.
Raises is not defined. I think you need to use the "new" pytest way of doing this:

def test_fill between_2d_x_input(): … with pytest.raises(ValueError): ax.plot(…)

anntzer · 2017-01-14T07:26:09Z

I would put the check much lower, after conversion to arrays (x = ma.masked_invalid(self.convert_xunits(x)) and so on). Then you can just check for x.ndim and so on.

anntzer · 2017-01-14T08:18:55Z

lib/matplotlib/axes/_axes.py

@@ -4774,6 +4775,11 @@ def fill_between(self, x, y1, y2=0, where=None, interpolate=False,
        y1 = ma.masked_invalid(self.convert_yunits(y1))
        y2 = ma.masked_invalid(self.convert_yunits(y2))

+        for array in [('x', x), ('y1', y1), ('y2', y2)]:


for name, array in ...

samsontmr · 2017-01-14T08:33:21Z

Done! @anntzer @NelleV

anntzer · 2017-01-14T09:49:47Z

lib/matplotlib/axes/_axes.py

@@ -4774,6 +4775,11 @@ def fill_between(self, x, y1, y2=0, where=None, interpolate=False,
        y1 = ma.masked_invalid(self.convert_yunits(y1))
        y2 = ma.masked_invalid(self.convert_yunits(y2))

+        for name, array in [('x', x), ('y1', y1), ('y2', y2)]:
+            if array.ndim > 1:
+                raise ValueError('Input passed into argument "' + name +


"Input passed into argument {!r}".format(name) (or "... %r" % name if you're really old-styled).
Same fixes need to be applied below.

NelleV

There is still a minor mistake that makes the tests fail.
Once this is fixed, it's good to go for me!

NelleV · 2017-01-14T19:07:55Z

lib/matplotlib/axes/_axes.py

@@ -4774,6 +4775,11 @@ def fill_between(self, x, y1, y2=0, where=None, interpolate=False,
        y1 = ma.masked_invalid(self.convert_yunits(y1))
        y2 = ma.masked_invalid(self.convert_yunits(y2))

+        for name, array in [('x', x), ('y1', y1), ('y2', y2)]:


NelleV · 2017-01-14T19:21:19Z

lib/matplotlib/axes/_axes.py

@@ -4934,6 +4941,11 @@ def fill_betweenx(self, y, x1, x2=0, where=None,
        x1 = ma.masked_invalid(self.convert_xunits(x1))
        x2 = ma.masked_invalid(self.convert_xunits(x2))

+        for array in [('x', x), ('y1', y1), ('y2', y2)]:


I think there is a slight mistake here: the arguments are "y", "x1" and "x2".
The documentation and examples don't run.

Good catch, thanks! Will change it now

NelleV · 2017-01-14T19:22:38Z

lib/matplotlib/axes/_axes.py

@@ -4934,6 +4941,11 @@ def fill_betweenx(self, y, x1, x2=0, where=None,
        x1 = ma.masked_invalid(self.convert_xunits(x1))
        x2 = ma.masked_invalid(self.convert_xunits(x2))

+        for array in [('x', x), ('y1', y1), ('y2', y2)]:


Any reason not to use name, array as above?

samsontmr · 2017-01-16T09:22:50Z

Added @cleanup at the top of the tests. I suspect that was what caused some of the build jobs to fail

codecov-io · 2017-01-16T10:02:03Z

Current coverage is 62.10% (diff: 66.66%)

Merging #7817 into master will increase coverage by <.01%

@@             master      #7817   diff @@
==========================================
  Files           174        174          
  Lines         56051      56057     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          34810      34815     +5   
- Misses        21241      21242     +1   
  Partials          0          0

Powered by Codecov. Last update b6709ea...e702cd8

tacaswell

I think all of @NelleV 's concerns have been addressed.

anntzer · 2017-01-18T23:05:22Z

Thanks!

samsontmr force-pushed the fix-#7510 branch from e1fc88d to 9a6c7ee Compare January 13, 2017 06:13

Add input validation for fill_between and fill_betweenx and unit tests

506bd99

samsontmr force-pushed the fix-#7510 branch from 9a6c7ee to 506bd99 Compare January 13, 2017 07:47

NelleV requested changes Jan 13, 2017

View reviewed changes

anntzer reviewed Jan 14, 2017

View reviewed changes

Move input validation to below conversion to array

c353bce

samsontmr force-pushed the fix-#7510 branch from 3b03392 to c353bce Compare January 14, 2017 08:23

Change tests to use pytest.raises() instead of @raises

72f6359

anntzer reviewed Jan 14, 2017

View reviewed changes

NelleV requested changes Jan 14, 2017

View reviewed changes

samsontmr force-pushed the fix-#7510 branch from 4c06839 to 7997b19 Compare January 15, 2017 00:28

tacaswell added this to the 2.1 (next point release) milestone Jan 15, 2017

samsontmr force-pushed the fix-#7510 branch from 7997b19 to f1d5d26 Compare January 15, 2017 02:56

Update exception string to use %r for formatting

e702cd8

samsontmr force-pushed the fix-#7510 branch from f1d5d26 to e702cd8 Compare January 16, 2017 08:37

tacaswell approved these changes Jan 16, 2017

View reviewed changes

tacaswell changed the title ~~better input validation on fill_between~~ [MRG+1] better input validation on fill_between Jan 16, 2017

anntzer merged commit 19a830e into matplotlib:master Jan 18, 2017

anntzer changed the title ~~[MRG+1] better input validation on fill_between~~ better input validation on fill_between Jan 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

better input validation on `fill_between` #7817

better input validation on `fill_between` #7817

samsontmr commented Jan 13, 2017

NelleV left a comment

NelleV Jan 13, 2017

anntzer commented Jan 14, 2017

anntzer Jan 14, 2017

samsontmr commented Jan 14, 2017

anntzer Jan 14, 2017

NelleV left a comment

NelleV Jan 14, 2017

NelleV Jan 14, 2017

samsontmr Jan 15, 2017

NelleV Jan 14, 2017

samsontmr commented Jan 16, 2017 •

edited

Loading

codecov-io commented Jan 16, 2017

tacaswell left a comment

anntzer commented Jan 18, 2017

		@@ -725,6 +725,42 @@ def test_polycollection_joinstyle():
		ax.set_ybound(0, 3)


		@raises(ValueError)

better input validation on fill_between #7817

better input validation on fill_between #7817

Conversation

samsontmr commented Jan 13, 2017

NelleV left a comment

Choose a reason for hiding this comment

NelleV Jan 13, 2017

Choose a reason for hiding this comment

anntzer commented Jan 14, 2017

anntzer Jan 14, 2017

Choose a reason for hiding this comment

samsontmr commented Jan 14, 2017

anntzer Jan 14, 2017

Choose a reason for hiding this comment

NelleV left a comment

Choose a reason for hiding this comment

NelleV Jan 14, 2017

Choose a reason for hiding this comment

NelleV Jan 14, 2017

Choose a reason for hiding this comment

samsontmr Jan 15, 2017

Choose a reason for hiding this comment

NelleV Jan 14, 2017

Choose a reason for hiding this comment

samsontmr commented Jan 16, 2017 • edited Loading

codecov-io commented Jan 16, 2017

Current coverage is 62.10% (diff: 66.66%)

tacaswell left a comment

Choose a reason for hiding this comment

anntzer commented Jan 18, 2017

better input validation on `fill_between` #7817

better input validation on `fill_between` #7817

samsontmr commented Jan 16, 2017 •

edited

Loading