-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
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: Handle overlapping line and bar on the same plot #61173
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Can you add a test in pandas/tests/plotting/test_series.py
. Make the plots and check that the x-axis is as expected.
pandas/plotting/_matplotlib/core.py
Outdated
@@ -1868,6 +1870,14 @@ def __init__( | |||
|
|||
MPLPlot.__init__(self, data, **kwargs) | |||
|
|||
if isinstance(data.index, ABCPeriodIndex): | |||
self.ax.freq = data.index.freq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the modification of freq necessary? I worry that we don't own ax
, and that we shouldn't be mutating it. But I'm not too familiar with the plotting code; do we do this anywhere else? I don't see any instances at a glance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason is to pass the freq to PeriodConverter, which currently only accepts ax as input arg, not freq. It can obviously be made cleaner. I meant to modify PeriodConverter or find a cleaner way than hacking around ax.
I'd just like to get a thorough review of the overall approach, i.e. shifting the tick positions, as it would apply to all box plots with PeriodIndex, and hence is a potential major change. Not sure how well it is covered by tests currently; I don't want to insert regressions or breaking changes. It would be good to have it first checked by a maintainer well-versed with matplotlib, as I'm not so familiar with all the idiosyncrasies of the ax object.
Introduce `convert_from_freq` method and streamline the conversion process by passing `freq` directly instead of using `axis.freq`. This improves modularity and ensures clearer separation of concerns for frequency handling in Period conversion.
Simplified `tick_pos` calculation by reusing helper methods and added a decorator to register pandas Matplotlib converters in the `_plot` method. This improves clarity and ensures proper integration with the pandas Matplotlib ecosystem.
This test ensures that bar and line plots with identical x values are correctly superposed on the same axes. It verifies that the x-tick positions remain consistent across plot types.
@@ -971,3 +971,17 @@ def test_secondary_y_subplot_axis_labels(self): | |||
s1.plot(ax=ax2) | |||
assert len(ax.xaxis.get_minor_ticks()) == 0 | |||
assert len(ax.get_xticklabels()) > 0 | |||
|
|||
def test_bar_line_plot(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails on main
Resolved a bug that prevented a line and bar from aligning on the same plot in `Series.plot`. This addresses issue pandas-dev#61161 and improves plot consistency when combining these chart types.
Move `x_compat` logic and time series helper methods from `LinePlot` to `MPLPlot` for better reusability and maintainability. This simplifies the `LinePlot` class and centralizes common functionality.
return PeriodConverter.convert_from_freq(values, axis.freq) | ||
|
||
@staticmethod | ||
def convert_from_freq(values, freq): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert
only uses the freq attribute of axis, so one should allow the user to pass freq without axis object.
pandas/plotting/_matplotlib/core.py
Outdated
from pandas.plotting import plot_params | ||
|
||
self.x_compat = plot_params["x_compat"] | ||
if "x_compat" in self.kwds: | ||
self.x_compat = bool(self.kwds.pop("x_compat")) | ||
|
||
@final | ||
def _is_ts_plot(self) -> bool: | ||
# this is slightly deceptive | ||
return not self.x_compat and self.use_index and self._use_dynamic_x() | ||
|
||
@final | ||
def _use_dynamic_x(self) -> bool: | ||
return use_dynamic_x(self._get_ax(0), self.data) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those routines are not exclusive to LinePlot, as they seem related to any type of time series. _is_ts_plot
is used in this PR to check if a boxplot is a time series.
@rhshadrach I covered the change with a test. I believe the PR is now ready for review and then merge. |
pandas/tests/plotting/test_series.py
Outdated
s.plot(kind="bar", ax=ax) | ||
bar_xticks = ax.get_xticks().tolist() | ||
s.plot(kind="line", ax=ax, color="r") | ||
line_xticks = ax.get_xticks()[: len(s)].tolist() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing ax.get_xticks()
here have the same length as s
; why have [: len(s)]
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After plotting the line, the ticks change on my machine:
ax.get_xticks()
Out[1]: array([53, 54, 55, 56])
ax.get_xticklabels()
Out[2]:
[Text(53, 0, '2023'),
Text(54, 0, '2024'),
Text(55, 0, '2025'),
Text(56, 0, '2026')]
A fourth tick is added by matplotlib for '2026'.
Even for integer indices, matplotlib does not do a 1-to-1 mapping between each x-tick and data point, as attested below:
ax = plt.subplot()
plt.plot(s.values, [1, 2,3])
ax.get_xticks()
array([0.75, 1. , 1.25, 1.5 , 1.75, 2. , 2.25, 2.5 , 2.75, 3. , 3.25])
Not sure if you can reproduce those results on your machine, but I think that the exact number of ticks is irrelevant. I would suggest that this test checks that both x-tick labels (after bar plot and after line plot) include at least the 3 three ticks in the index (2023, 2024, 2025) and that the associated x-tick positions are the same for both plots. This would ensure that the two plots are superposed.
Additionally, the test can check that x-lim includes those three x-tick positions, which would ensure that the current plot view render the two plots.
I added a commit to reflect what I said above. Happy to discuss or update the code further if there is a need.
# Conflicts: # doc/source/whatsnew/v3.0.0.rst # pandas/plotting/_matplotlib/core.py
Ensure bar and line plots share consistent x-axis tick labels and verify that x-axis limits are adjusted to make all plotted elements visible in `test_bar_line_plot` test. These changes improve the robustness of visual alignment and boundary checks.
Add an assertion to verify the length of `bar_xticks` aligns with the length of the index. This improves the test's robustness by ensuring the data and ticks remain consistent.
assert len(bar_xticks) == len(index) | ||
assert bar_xticks == line_xticks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checking that
- the three index years are in the two plots
- the x-tick positions for those years are the same in the two plots
assert x_limits[0] <= bar_xticks[0].get_position()[0] | ||
assert x_limits[1] >= bar_xticks[-1].get_position()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check that both left-most and right-most data points are shown in the current view
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.This code
from the linked issue gives this plot:

And swapping the lines (line before bar)
gives

I believe the difference of results stems from the fact that the figure is always cropped to match the x-range of the last plot. We can certainly adjust the PR to make the first figure behave like the second one if desired.
Please provide feedback on the overall approach. Once confirmed, I'll expand and cover with tests.