Align y-axes in mmm.plot.budget_allocation#1967
Align y-axes in mmm.plot.budget_allocation#1967TeemuSailynoja wants to merge 10 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1967 +/- ##
==========================================
+ Coverage 93.05% 93.08% +0.03%
==========================================
Files 78 78
Lines 12230 12256 +26
==========================================
+ Hits 11381 11409 +28
+ Misses 849 847 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
still needs tests to go through the combinations of negative positive and ensure that the axis limits are correct after the call. |
pymc_marketing/mmm/plot.py
Outdated
| if ax.axes.get_ylim()[0] < 0 or ax2.axes.get_ylim()[0] < 0: | ||
| ylims1 = ax.axes.get_ylim() | ||
| ylims2 = ax2.axes.get_ylim() | ||
| # Find the ratio of negative vs. positive part of the axes. | ||
| if ylims1[1]: | ||
| ax1_yratio = ylims1[0] / ylims1[1] | ||
| else: | ||
| # Fully negative axis. | ||
| ax1_yratio = -1 | ||
|
|
||
| if ylims2[1]: | ||
| ax2_yratio = ylims2[0] / ylims2[1] | ||
| else: | ||
| # Fully negative axis, may need to reflect the other | ||
| ax2_yratio = -1 | ||
|
|
||
| # Make axis adjustments. If both axes fully negative, no adjustment. | ||
| if ax1_yratio < ax2_yratio: | ||
| ax2.set_ylim(bottom=ylims2[1] * ax1_yratio) | ||
| if ax1_yratio == -1: | ||
| # if the axis is fully negative, center zero. | ||
| ax.set_ylim(top=-ylims1[0]) | ||
| elif ax2_yratio < ax1_yratio: | ||
| ax.set_ylim(bottom=ylims1[1] * ax2_yratio) | ||
| if ax2_yratio == -1: | ||
| # if the axis is fully negative, center zero. | ||
| ax2.set_ylim(top=-ylims2[0]) |
There was a problem hiding this comment.
Can you use NamedTuple or something. Quite confusing to read
There was a problem hiding this comment.
Looking at this now, I fully agree. I also think that this is not capturing every possible combination the axes can get odd. I'll move this to a draft and rework it.
There was a problem hiding this comment.
I completely rewrote the function, and added tests that go through the possible cases.
Not the most compact solution, but I hope it exposes its logic better.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
github-actions
bot
added
docs
TeemuSailynoja
force-pushed
the
TeemuSailynoja/plot-budget-allocation-align-y-axes
branch
from
2a16e3d to
772756f
Compare
…Sailynoja/plot-budget-allocation-align-y-axes
TeemuSailynoja
added
plots
and removed
docs
pymc_marketing/mmm/plot.py
Outdated
| return ", ".join(title_parts) | ||
| return fallback_title | ||
|
|
||
| def _align_y_axes(self, ax_left, ax_right) -> None: |
There was a problem hiding this comment.
@TeemuSailynoja can you please add type hinds and complete docstrings to this function so that we can bring it to the finish line :) Thanks!
There was a problem hiding this comment.
@juanitorduz I'm sorry for the delay. Work got in the way 😝
I hope what I added is good. Thank you for pingin me on this!
PR SummaryLow Risk Overview Adds Written by Cursor Bugbot for commit ef8e62a. This will update automatically on new commits. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # Right axis is all positive, left axis has negative values | ||
| ax_right.set_ylim( | ||
| bottom=-zero_rel_pos_left * ylims_right.top / (1 - zero_rel_pos_left) | ||
| ) |
There was a problem hiding this comment.
Zero position miscomputed when 0 off-scale
Medium Severity
_align_y_axes treats zero_rel_pos_* as only being in {0, 1} or “mixed”, but when an axis range doesn’t include 0 (e.g., fully negative with top < 0 or fully positive with bottom > 0), zero_rel_pos_* becomes outside [0, 1] and the “mixed” branch can run, producing incorrect or inverted ylim adjustments.
| zero_rel_pos_left = -ylims_left.bottom / (ylims_left.top - ylims_left.bottom) | ||
| zero_rel_pos_right = -ylims_right.bottom / ( | ||
| ylims_right.top - ylims_right.bottom | ||
| ) |
There was a problem hiding this comment.
Division by zero for flat y-limits
Medium Severity
_align_y_axes divides by (top - bottom) when computing zero_rel_pos_*. If an axis has identical limits (possible with constant/empty data or manual set_ylim((c, c))), this becomes a division by zero and can propagate inf/nan into subsequent set_ylim calls.
3 participants
Description
Ensure that y=0 is aligned between the primary and secondary y-axis in
mmm.plot.budget_allocation.Solution
If one axis includes negative, values the other is extended down so that the y=0 is aligned.
If all contributions would be fully negative (I don't know when this would happen...), the y=0 level is centered vertically on both axes.
Related Issue
Checklist
pre-commit.ci autofixto auto-fix.📚 Documentation preview 📚: https://pymc-marketing--1967.org.readthedocs.build/en/1967/