BUG: bar and line plots are not aligned on the x-axis/xticks

[sharonwoo]

• closes BUG: bar a line plots are not aligned on the x-axis/xticks #56460
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

Added a test which will fail on main's self.tick_pos = np.arange(len(data)), which in this test will result in actual_bar_x of [0,1,2,3,4].

def test_bar_plot_x_axis(self, plot_data):
    df = DataFrame(plot_data)
    ax_bar = df["bars"].plot(kind="bar")
    df["pct"].plot(kind="line")
    actual_bar_x = [bar.get_x() + bar.get_width() / 2.0 for bar in ax_bar.patches]
    expected_x = [-1, 0, 1, 2, 3]
    assert actual_bar_x == expected_x
        ```

[sharonwoo]

@rhshadrach or anyone interested in reviewing, welcoming comments

[WillAyd]

Thanks for submitting a PR

[WillAyd]

This part of the code base is not my area of expertise but what does ax.get_x() return here? I'm a bit surprised we have to do this much work to get the labels

[WillAyd]

From the OP it seems like the issue was LinePlot not having the proper labels - is that not the case? If so I think we need to change the logic in that class or maybe even remove it.

[sharonwoo]

Hi, thanks for the comment.

I think the related issue here explains it better than I possibly can: #55508

This is my attempt to explain:

When a BarPlot and a LinePlot are combined, because BarPlot sets sequential indices starting from 0 (the line of code: self.tick_pos = np.arange(len(data))) while LinePlot uses what it gets, the combined plot uses the correct labels on BarPlot's incorrect xticks.

If the index starts from 3 instead, the line plot is "shifted" by 3 and partially cut off: #56460 (comment)

(I think the displayed labels are reconciled in ticker._compute_offset with self.locs containing the entire range when it enters, and then not when it exits, but I am not 100% sure as it took quite a bit of stepping through the code to get there and I was interrupted by Christmas festivities around that time.)

[rhshadrach]

Thanks for the PR! It appears to me that if the xticks are not numeric, the current approach here will fail.

[rhshadrach]

It seems like this would fail on the input:

df = pd.DataFrame(
    {
        "bars": {
            "a": 0.5,
            "b": 1.0,
        },
        "pct": {
            "a": 4.0,
            "b": 2.0,
        },
    }
)

Is that correct?

[sharonwoo]

Yes, thanks for catching this. That would fail completely.

In the current code, that dataframe would return a graph with xticks of [0,1], since BarPlot currently sets a numpy integer array for xticks with self.tick_pos = np.arange(len(data)).

In the context of this bug, using is_integer_dtype(data.index) should handle the above case within the confines of the integer array BarPlot expects:

def _set_tick_pos(self, data) -> np.ndarray:
    # only set the tick_pos to the index, if index is an integer series
    # else return a data-length array of integers 
    if is_integer_dtype(data.index) and self._is_series:
        return data.index
    else:
        return np.arange(len(data))

Would it, however, be a good idea to expand the scope of this bug fix, to encompass also

1. When the index is a timestamp
2. When the index is a set of floats

This would not so much be a bug fix as a change in the way BarPlot handles plotting, though. As a user, I have always just reset_index() in order to handle these cases. Thoughts?

[sharonwoo]

I saw your comments on the bug report, I also need to address them.

[sharonwoo]

as is, this test expects [1,2,3,4] and subsequently [10, 50, 20, 30]. it actually passes on both current BarPlot and PR implementations.

but on the actual displayed graph, the xticks are sorted in ascending order. to investigate what's causing this behaviour and to fix the test (or create a new one to test fixed logic)

[github-actions]

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

[sharonwoo]

I think this fix is actually irrelevant now because of the discussion in the issue.

[rhshadrach]

@sharonwoo - no problem, will close for now. If you'd like to take up the fix discussed in the issue, feel free to either reuse this PR (we can reopen) or open a new one.