BUG: Plotting Timedelta on y-axis #16953 #17430

s-weigand · 2017-09-04T19:07:43Z

closes Plotting Timedelta on y-axis #16953
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

This fixes the issue with a TypeError being raised if the y-data were of type Timedelta or Datetime.
The plot of Timedelta doesn't look pretty since it is converted to ns representation, but matplotlib has already an open issue for that.

PS: Greetings to the people from the EuroSciPy 2017 sprints ^^

gfyoung · 2017-09-04T21:17:40Z

@s-weigand : Don't forget a whatsnew for this PR!

codecov · 2017-09-04T21:55:29Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17430      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files         163      163              
  Lines       49581    49582       +1     
==========================================
- Hits        45198    45191       -7     
- Misses       4383     4391       +8

Flag	Coverage Δ
#multiple	`88.92% <100%> (ø)`	⬆️
#single	`40.25% <0%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`82.69% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.72% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.43% <0%> (+0.09%)`	⬆️

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 1981b67...13b60dc. Read the comment docs.

codecov · 2017-09-04T21:55:46Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17430      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files         163      163              
  Lines       49581    49582       +1     
==========================================
- Hits        45198    45191       -7     
- Misses       4383     4391       +8

Flag	Coverage Δ
#multiple	`88.92% <100%> (ø)`	⬆️
#single	`40.25% <0%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/plotting/_core.py	`82.69% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.72% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.43% <0%> (+0.09%)`	⬆️

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 1981b67...1ebfb13. Read the comment docs.

… object dtypes

pep8speaks · 2017-09-04T23:33:17Z

Hello @s-weigand! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on September 05, 2017 at 17:38 Hours UTC

jreback · 2017-09-04T23:34:34Z

pandas/plotting/_core.py

@@ -342,7 +342,9 @@ def _compute_plot_data(self):
                label = 'None'
            data = data.to_frame(name=label)

-        numeric_data = data._convert(datetime=True)._get_numeric_data()
+        # GH16953
+        data = data._convert(datetime=True, timedelta=True)


I don't think we need the _convert at all here. this is for object types that need to be coerced. almost all routines in pandas already assume this.

The _convert is needed for a test, where a Series with numeric values and dtype object is used (tests.plotting.test_series.test_valid_object_plot).
If _convert isn't used, that test will fail in circleci.
To prevent the test from failing, _convert can either be used only on Series with dtype==object and after the to_frame (line 344), or on data in general after the Series specific part (line 346 as is now).

jreback · 2017-09-04T23:35:18Z

doc/source/whatsnew/v0.21.0.txt

@@ -400,7 +400,7 @@ I/O
 Plotting
 ^^^^^^^^
 - Bug in plotting methods using ``secondary_y`` and ``fontsize`` not setting secondary axis font size (:issue:`12565`)
-
+- Bug when plotting Timedelta and Datetime on y-axis (:issue:`16953`)


say timedelta and datetime dtypes (and use double back-ticks).

jreback · 2017-09-04T23:37:22Z

pandas/tests/plotting/test_frame.py

+                "timedelta": [pd.Timedelta(10, unit="s"),
+                              pd.Timedelta(10, unit="m"),
+                              pd.Timedelta(10, unit="h")],
+                "datetime": [pd.to_datetime("2017-08-01 00:00:00"),


can you add a datetime with a tz as well

I think some other types, e.g. Period might fail, if you can add them in another test with xfail would be fine (Categoricals might work as they area converted to ndarrays)

jreback · 2017-09-05T17:16:01Z

pandas/tests/plotting/test_frame.py

+        with pytest.raises(TypeError):
+            testdata.plot(y="text")
+
+    @pytest.mark.xfail


you need to have a reason='not support for period, categorical, datetime_mixed_tz') here

jreback · 2017-09-05T17:16:49Z

pandas/plotting/_core.py

-        numeric_data = data._convert(datetime=True)._get_numeric_data()
+        # GH16953, _convert is needed as fallback, for ``Series``
+        # with ``dtype == object``
+        data = data._convert(datetime=True, timedelta=True)


did you take out the _convert and see, on what input does it fail?

Yap i did, circleci fails at tests.plotting.test_series.test_valid_object_plot, since the Series is of dtype == object (s = Series(lrange(10), dtype=object), also see comment on your last requested changes).
Also i think doing a general data._convert is more sturdy, than just calling in if the Seriesis of type object (which would be the minimal change for the test to pass).

ok I guess leave it in is ok. In general we don't automatically coerce object types (even if they are numbers) in other functions / places; we happen to do it in plotting for compat I think. Can you open a new issue for this to discuss (deprecating this).

jreback · 2017-09-06T11:59:06Z

lgtm. @TomAugspurger

TomAugspurger · 2017-09-06T12:03:48Z

Thanks @s-weigand!

s-celles · 2017-09-06T14:04:13Z

@s-weigand What is matplotlib issue number? Thanks for this fix.

* implemented fix for GH issue pandas-dev#16953 * added tests for fix of issue pandas-dev#16953 * changed comments for git issue to pandas style GH# * changed linelength in tests, so all lines are less than 80 characters * added whatsnew entry * swaped conversion and filtering of values, for plot to also work with object dtypes * refomated code, so len(line) < 80 * changed whatsnew with timedelta and datetime dtypes * added support for datetimetz and extended tests * added reason to pytest.mark.xfail

s-weigand added 4 commits September 4, 2017 18:15

implemented fix for GH issue pandas-dev#16953

84a8cc2

added tests for fix of issue pandas-dev#16953

f8dcb84

changed comments for git issue to pandas style GH#

23475c2

changed linelength in tests, so all lines are less than 80 characters

9bb7389

gfyoung added the Visualization plotting label Sep 4, 2017

added whatsnew entry

13b60dc

swaped conversion and filtering of values, for plot to also work with…

eb6a110

… object dtypes

refomated code, so len(line) < 80

c438bb2

jreback added this to the 0.21.0 milestone Sep 4, 2017

jreback requested changes Sep 4, 2017

View reviewed changes

jreback reviewed Sep 4, 2017

View reviewed changes

s-weigand added 2 commits September 5, 2017 17:56

changed whatsnew with timedelta and datetime dtypes

e51a558

added support for datetimetz and extended tests

84ad1d4

jreback reviewed Sep 5, 2017

View reviewed changes

added reason to pytest.mark.xfail

1ebfb13

jreback approved these changes Sep 6, 2017

View reviewed changes

TomAugspurger merged commit 25d5299 into pandas-dev:master Sep 6, 2017

jorisvandenbossche mentioned this pull request Sep 15, 2017

Can't plot pd.Timedelta (or numpy.timedelta64) versus time #9154

Closed

This was referenced Sep 15, 2017

Improve plotting Timedelta on y-axis (Timedelta string representation) #17553

Open

Plotting Timedelta on y-axis #16953

Closed

s-weigand deleted the fix_issue_#16953 branch October 15, 2017 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Plotting Timedelta on y-axis #16953 #17430

BUG: Plotting Timedelta on y-axis #16953 #17430

s-weigand commented Sep 4, 2017

gfyoung commented Sep 4, 2017

codecov bot commented Sep 4, 2017

codecov bot commented Sep 4, 2017 •

edited

Loading

pep8speaks commented Sep 4, 2017 •

edited

Loading

jreback Sep 4, 2017

s-weigand Sep 5, 2017

jreback Sep 4, 2017

jreback Sep 4, 2017

s-weigand Sep 4, 2017

jreback Sep 4, 2017

jreback Sep 5, 2017

jreback Sep 5, 2017

s-weigand Sep 5, 2017

jreback Sep 5, 2017

jreback commented Sep 6, 2017

TomAugspurger commented Sep 6, 2017

s-celles commented Sep 6, 2017

BUG: Plotting Timedelta on y-axis #16953 #17430

BUG: Plotting Timedelta on y-axis #16953 #17430

Conversation

s-weigand commented Sep 4, 2017

gfyoung commented Sep 4, 2017

codecov bot commented Sep 4, 2017

Codecov Report

codecov bot commented Sep 4, 2017 • edited Loading

Codecov Report

pep8speaks commented Sep 4, 2017 • edited Loading

Comment last updated on September 05, 2017 at 17:38 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 6, 2017

TomAugspurger commented Sep 6, 2017

s-celles commented Sep 6, 2017

codecov bot commented Sep 4, 2017 •

edited

Loading

pep8speaks commented Sep 4, 2017 •

edited

Loading