Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: xticks unnecessarily rotated #34334

Merged
merged 32 commits into from
Sep 13, 2020

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented May 23, 2020

On master, xticklabels are rotated if

            not self._use_dynamic_x()
            and data.index.is_all_dates
            and not self.subplots
            or (self.subplots and self.sharex)

There's a comment saying

        if condition:
            # irregular TS rotated 30 deg. by default
            # probably a better place to check / set this.
            if not self._rot_set:
                self.rot = 30

, and it seems that the intention was to rotate ticklabels if there's an irregular timeseries.

With the condition as it is written, they will also be rotated if the following is true:

(self.subplots and self.sharex)

I don't see why that should warrant rotating the ticklabels, so I'd like to suggest that there was a bracketing error.

Also, the condition data.index.is_all_dates is (as far as I can tell) only relevant if use_index=True, so I've changed that.

Here's what the OP's plot looks like with these changes:

image

Here's what it looks like on master:

image

Irregular time series still have their ticklabels rotated:
image

Regular time series' ticklabels don't rotate:
image

(unless you pass xcompat=True, but that's unaffected by this change)

@MarcoGorelli MarcoGorelli added the Visualization plotting label May 23, 2020
@MarcoGorelli MarcoGorelli marked this pull request as draft June 12, 2020 13:09
@MarcoGorelli MarcoGorelli marked this pull request as ready for review June 18, 2020 06:28
Copy link
Member

@charlesdong1991 charlesdong1991 left a comment

Choose a reason for hiding this comment

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

very nice!

only one nitpick, but could also choose to ignore since it doesn't really matter

Comment on lines 1497 to 1520
x = to_datetime(["2020-05-01", "2020-05-02", "2020-05-04"])
df = DataFrame({"x": x, "y": [1, 2, 3]})
axes = df.plot(x="x", y="y")
self._check_ticks_props(axes, xrot=30)

# use timeseries index
axes = df.set_index("x").plot(y="y", use_index=True)
self._check_ticks_props(axes, xrot=30)

# separate subplots
axes = df.plot(x="x", y="y", subplots=True, sharex=True)
self._check_ticks_props(axes, xrot=30)
axes = df.plot(x="x", y="y", subplots=True, sharex=False)
self._check_ticks_props(axes, xrot=0)

# index of dates but don't use index
axes = df.set_index("x").plot(y="y", use_index=False)
self._check_ticks_props(axes, xrot=0)

# regular time series
x = to_datetime(["2020-05-01", "2020-05-02", "2020-05-03"])
df = DataFrame({"x": x, "y": [1, 2, 3]})
axes = df.plot(x="x", y="y")
self._check_ticks_props(axes, xrot=0)
Copy link
Member

@charlesdong1991 charlesdong1991 Jun 18, 2020

Choose a reason for hiding this comment

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

not a big issue, i would prefer to shuffle the tests a bit to put similar tests together because it's quite clear there are three sub-groups to test, I would prefer something like this:

        # regular time series
        x = to_datetime(["2020-05-01", "2020-05-02", "2020-05-03"])
        df = DataFrame({"x": x, "y": [1, 2, 3]})
        axes = df.plot(x="x", y="y")
        self._check_ticks_props(axes, xrot=0)

        # irregular time series
        x = to_datetime(["2020-05-01", "2020-05-02", "2020-05-04"])
        df = DataFrame({"x": x, "y": [1, 2, 3]})
        axes = df.plot(x="x", y="y")
        self._check_ticks_props(axes, xrot=30)

        # use timeseries index or not
        axes = df.set_index("x").plot(y="y", use_index=True)
        self._check_ticks_props(axes, xrot=30)
        axes = df.set_index("x").plot(y="y", use_index=False)
        self._check_ticks_props(axes, xrot=0)

        # separate subplots
        axes = df.plot(x="x", y="y", subplots=True, sharex=True)
        self._check_ticks_props(axes, xrot=30)
        axes = df.plot(x="x", y="y", subplots=True, sharex=False)
        self._check_ticks_props(axes, xrot=0)

or even to split them into different tests, and parametrize the parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good suggestion, thanks!

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can u merge master and ping on green

@MarcoGorelli
Copy link
Member Author

@jreback ping

@@ -1099,6 +1099,7 @@ Plotting
- Bug in :meth:`DataFrame.boxplot` and :meth:`DataFrame.plot.boxplot` lost color attributes of ``medianprops``, ``whiskerprops``, ``capprops`` and ``medianprops`` (:issue:`30346`)
- Bug in :meth:`DataFrame.hist` where the order of ``column`` argument was ignored (:issue:`29235`)
- Bug in :meth:`DataFrame.plot.scatter` that when adding multiple plots with different ``cmap``, colorbars alway use the first ``cmap`` (:issue:`33389`)
- Bug in :meth:`Dataframe.plot` was rotating xticklabels when subplots was equal to True, even if the x-axis wasn't an irregular time series (:issue:`29460`)
Copy link
Member

@charlesdong1991 charlesdong1991 Jul 17, 2020

Choose a reason for hiding this comment

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

i am very sorry to be a badass to block your PR, just come across it, can you change Dataframe to DataFrame? 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

😆 good catch, thanks!

@simonjayhawkins
Copy link
Member

@MarcoGorelli can you move release note to 1.2

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

tiny comment, otherwsie lgtm

@@ -239,7 +239,7 @@ I/O
Plotting
^^^^^^^^

-
- Bug in :meth:`DataFrame.plot` was rotating xticklabels when subplots was equal to True, even if the x-axis wasn't an irregular time series (:issue:`29460`)
Copy link
Contributor

Choose a reason for hiding this comment

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

use subplots=True (in double-backticks) so its easily readable

@jreback
Copy link
Contributor

jreback commented Aug 12, 2020

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Aug 17, 2020 via email

Copy link
Member

@charlesdong1991 charlesdong1991 left a comment

Choose a reason for hiding this comment

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

lgtm

@jbrockmendel
Copy link
Member

@MarcoGorelli can you rebase. looks like this was about ready

@jreback jreback added this to the 1.2 milestone Sep 13, 2020
@jreback jreback merged commit 4c4db19 into pandas-dev:master Sep 13, 2020
@jreback
Copy link
Contributor

jreback commented Sep 13, 2020

thanks for this @MarcoGorelli nice! (and good to add the testing like you did).

@MarcoGorelli MarcoGorelli deleted the subplots-non-date branch September 14, 2020 07:25
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xticks unnecessarily rotated
6 participants