API: Improper x/y arg given to df.plot #18695

masongallo · 2017-12-08T16:52:11Z

closes UserWarning about columns and attribute while plotting in Jupyter #18671
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I'm not incredibly familiar with your codebase so I did my best to follow conventions. Not sure what to do with whatsnew for this case?

Description:

Validation of x or y arg to df.plot to match specifications in documentation

TomAugspurger · 2017-12-08T17:01:00Z

Thanks, this will be for 0.22 since it's an API change. Some people may have been ignoring the warning and relying on this.

Did you look into how difficult it'd be to properly support multiple columns for y? At the very least, we would need to make sure that label was a a list-like of the same length as y. Not sure what else.

masongallo · 2017-12-08T17:16:35Z

Did you look into how difficult it'd be to properly support multiple columns for y?

I did think about this - the usecase that isn't already covered wasn't clear to me, especially given the complexity it would add to the method/API. The plot method already has subplots and secondary_y for example. What do you think?

TomAugspurger · 2017-12-08T17:19:09Z

I don't have a strong opinion. I think the equivalent output would be df.set_index("x")[y].plot(), which isn't so bad. It's probably best to just assert that y is a a single column, so your changes here look good.

The release not can go in doc/source/whatsnew/v0.22.0.txt under plotting.

masongallo · 2017-12-08T17:32:22Z

The release not can go in doc/source/whatsnew/v0.22.0.txt under plotting.

Thanks, I see api changes section will update that

codecov · 2017-12-08T18:00:05Z

Codecov Report

Merging #18695 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18695      +/-   ##
==========================================
- Coverage    91.6%   91.59%   -0.02%     
==========================================
  Files         153      153              
  Lines       51272    51276       +4     
==========================================
- Hits        46967    46964       -3     
- Misses       4305     4312       +7

Flag	Coverage Δ
#multiple	`89.45% <100%> (ø)`	⬆️
#single	`40.67% <0%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`82.42% <100%> (+0.05%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/util/testing.py	`82.01% <0%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 288bf6e...e190aa3. Read the comment docs.

codecov · 2017-12-08T18:00:06Z

Codecov Report

Merging #18695 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18695      +/-   ##
==========================================
- Coverage    91.6%   91.59%   -0.02%     
==========================================
  Files         153      153              
  Lines       51272    51276       +4     
==========================================
- Hits        46967    46964       -3     
- Misses       4305     4312       +7

Flag	Coverage Δ
#multiple	`89.45% <100%> (ø)`	⬆️
#single	`40.68% <12.5%> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`82.41% <100%> (+0.03%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/io/excel.py	`90.13% <0%> (ø)`	⬆️
pandas/core/internals.py	`94.44% <0%> (ø)`	⬆️
pandas/io/parsers.py	`95.55% <0%> (ø)`	⬆️
pandas/util/testing.py	`82.01% <0%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 288bf6e...aea8383. Read the comment docs.

jreback · 2017-12-09T15:28:26Z

pandas/tests/plotting/test_frame.py

@@ -2170,6 +2170,15 @@ def test_invalid_kind(self):
        with pytest.raises(ValueError):
            df.plot(kind='aasdf')

+    def test_invalid_xy_args(self):
+        df = DataFrame({"A": [1, 2], 'B': [3, 4], 'C': [5, 6]})


can yu add the issue number here

jreback · 2017-12-09T15:28:56Z

pandas/tests/plotting/test_frame.py

@@ -2170,6 +2170,15 @@ def test_invalid_kind(self):
        with pytest.raises(ValueError):
            df.plot(kind='aasdf')

+    def test_invalid_xy_args(self):


can you parameterize this on x, y

jreback · 2017-12-09T15:31:38Z

pandas/tests/plotting/test_frame.py

+        df = DataFrame({"A": [1, 2], 'B': [3, 4], 'C': [5, 6]})
+        bad_arg = ['B', 'C']
+        valid_arg = 'A'
+        with pytest.raises(ValueError):


can you also add a dataframe with duplicate columns as a test, e.g.

In [6]: df = DataFrame([[1, 3, 5], [2, 4, 6]], columns=list('AAB')) In [7]: df Out[7]: A A B 0 1 3 5 1 2 4 6 In [8]: df['A'] Out[8]: A A 0 1 3 1 2 4

jreback · 2017-12-09T15:32:21Z

doc/source/whatsnew/v0.22.0.txt

@@ -188,6 +188,7 @@ Other API Changes
 - :func:`pandas.DataFrame.merge` no longer casts a ``float`` column to ``object`` when merging on ``int`` and ``float`` columns (:issue:`16572`)
 - The default NA value for :class:`UInt64Index` has changed from 0 to ``NaN``, which impacts methods that mask with NA, such as ``UInt64Index.where()`` (:issue:`18398`)
 - Refactored ``setup.py`` to use ``find_packages`` instead of explicitly listing out all subpackages (:issue:`18535`)
+- :func: `DataFrame.plot` now raises a ``ValueError`` when the ``x`` or ``y`` argument is improperly formed (:issue:`18671`)


you can move this to plotting bugs.

jreback · 2017-12-09T15:33:31Z

pandas/plotting/_core.py

@@ -1706,11 +1706,15 @@ def _plot(data, x=None, y=None, subplots=False,
            if x is not None:
                if is_integer(x) and not data.columns.holds_integer():
                    x = data.columns[x]
+                elif not isinstance(data[x], Series):


from pandas.core.dtypes.generic import ABCSeries, ABCDataFrame

not isinstance(data[x], ABCSeries)

and change the DataFrame tests to use ABCDataFrame (and remove the inline import)

and change the DataFrame tests to use ABCDataFrame (and remove the inline import)

Not sure what you mean here? I changed the test to use ABCDataFrame but that doesn't seem right?

jreback · 2017-12-10T15:40:08Z

thanks @masongallo nice patch! keep em coming!

flutefreak7 · 2018-03-20T18:29:11Z

pandas/plotting/_core.py

                data = data.set_index(x)

            if y is not None:
                if is_integer(y) and not data.columns.holds_integer():
                    y = data.columns[y]
+                elif not isinstance(data[y], Series):


So if I do df.plot.line(x='x', y=['y1', 'y2', 'y3']) this is now a ValueError?

TomAugspurger · 2018-03-20T18:30:28Z

See #19699

…

On Tue, Mar 20, 2018 at 1:29 PM, flutefreak7 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/plotting/_core.py <#18695 (comment)>: > data = data.set_index(x) if y is not None: if is_integer(y) and not data.columns.holds_integer(): y = data.columns[y] + elif not isinstance(data[y], Series): So if I do df.plot.line(x='x', y=['y1', 'y2', 'y3']) this is now a ValueError? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#18695 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIlLuQemvhx7IqH8azMnUaXdQQf1Qks5tgUqAgaJpZM4Q7XF0> .

Raise ValueError if improper argument given to df.plot

bd01a37

update whatsnew 0.22

e190aa3

jreback requested changes Dec 9, 2017

View reviewed changes

jreback added Bug Visualization plotting labels Dec 9, 2017

jreback requested changes Dec 9, 2017

View reviewed changes

masongallo added 2 commits December 9, 2017 16:30

address fdbck

c5f3e2a

hopefully correct abcdataframe

aea8383

jreback added this to the 0.22.0 milestone Dec 10, 2017

jreback approved these changes Dec 10, 2017

View reviewed changes

jreback merged commit d7d8f2d into pandas-dev:master Dec 10, 2017

TomAugspurger mentioned this pull request Dec 13, 2017

warning in bar plot with multiple columns #18764

Closed

TomAugspurger mentioned this pull request Feb 6, 2018

Bug: different colors from plot and legend using groupby and unstack. #19544

Open

TomAugspurger mentioned this pull request Feb 14, 2018

Allow list-like for y in DataFrame.plot. #19699

Closed

flutefreak7 reviewed Mar 20, 2018

View reviewed changes

soxofaan mentioned this pull request Jun 8, 2018

regression: bar plot with multi-column category doesn't work anymore #21386

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Improper x/y arg given to df.plot #18695

API: Improper x/y arg given to df.plot #18695

masongallo commented Dec 8, 2017 •

edited

Loading

TomAugspurger commented Dec 8, 2017

masongallo commented Dec 8, 2017

TomAugspurger commented Dec 8, 2017

masongallo commented Dec 8, 2017

codecov bot commented Dec 8, 2017

codecov bot commented Dec 8, 2017 •

edited

Loading

jreback Dec 9, 2017

jreback Dec 9, 2017

jreback Dec 9, 2017

jreback Dec 9, 2017

jreback Dec 9, 2017

masongallo Dec 10, 2017

jreback commented Dec 10, 2017

flutefreak7 Mar 20, 2018

TomAugspurger commented Mar 20, 2018 via email

API: Improper x/y arg given to df.plot #18695

API: Improper x/y arg given to df.plot #18695

Conversation

masongallo commented Dec 8, 2017 • edited Loading

TomAugspurger commented Dec 8, 2017

masongallo commented Dec 8, 2017

TomAugspurger commented Dec 8, 2017

masongallo commented Dec 8, 2017

codecov bot commented Dec 8, 2017

Codecov Report

codecov bot commented Dec 8, 2017 • edited Loading

Codecov Report

jreback Dec 9, 2017

Choose a reason for hiding this comment

jreback Dec 9, 2017

Choose a reason for hiding this comment

jreback Dec 9, 2017

Choose a reason for hiding this comment

jreback Dec 9, 2017

Choose a reason for hiding this comment

jreback Dec 9, 2017

Choose a reason for hiding this comment

masongallo Dec 10, 2017

Choose a reason for hiding this comment

jreback commented Dec 10, 2017

flutefreak7 Mar 20, 2018

Choose a reason for hiding this comment

TomAugspurger commented Mar 20, 2018 via email

masongallo commented Dec 8, 2017 •

edited

Loading

codecov bot commented Dec 8, 2017 •

edited

Loading