-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[Bug]: align
in HPacker
is reversed
#24386
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
Comments
This indeed seems incorrect, however, I'm not sure what the path to fixing it is even that it's likely been that way for quite a while and swapping back will break anyone who had corrected for the mistake. I can't see that we use this internally, and it's obviously untested. |
There's no easy migration path. Probably the simplest thing is
This is a bit annoying on the user-side because they need two steps (1) invert their logic and add the flag (2) remove the flag. But one cannot do it any simpler if one does not want to make a hard break, which is not an option. |
I guess the fact we never use this internally, and no one has complained so far, indicates to me that this isn't used much externally? If so, perhaps we just fix it? |
Why doesn't this count as a behavior change API change? We do it rarely but we have kinda documented process for it? |
For reference, the |
Introducing a flag like fixed_api and later removing it is quite a bit of work both for us and on the end user's side; one option that would be a bit less annoying (requiring handing this over fewer versions on our side and requiring fewer changes from the end users) would be to
|
That works and is a shorter route at the cost of having subtle and IMHO ugly replacement values. A somewhat drastic approach is write new layout classes and deprecate the packers. In the basic form that could be the same content but with fixed behavior and a new class name. Or we could use the occasion to create more capable alignment, c.f. #23140 (comment). |
During the call it was also suggested by @greglucas (IIRC) that this should just be considered a plain bugfix, which is also an option I'm warming up to... (perhaps that's also @jklymak's opinion stated just above.) |
My argument is that if we make a mistake and accidentally redefine something like: We have a user here who identified this bug and would find use in us fixing it properly, so why dance around fixing it with a long deprecation that this user will now have to work around themselves before the proper fix is in by default? I don't think this is a clear-cut case either way for how to proceed, but I did want to bring up this option of calling this a bug rather than a "feature" that someone else has relied upon. This code goes way back to svn in 2008 3ae9221, and perhaps doesn't get used in the bottom/top mode much because our offsetbox and legend test cases don't fail when moving the bottom/top definitions around. Just to be clear, I think this is the patch we are all talking about: diff --git a/lib/matplotlib/offsetbox.py b/lib/matplotlib/offsetbox.py
index 89bd3550f3..fcad63362b 100644
--- a/lib/matplotlib/offsetbox.py
+++ b/lib/matplotlib/offsetbox.py
@@ -170,10 +170,10 @@ def _get_aligned_offsets(hd_list, height, align="baseline"):
descent = max(d for h, d in hd_list)
height = height_descent + descent
offsets = [0. for h, d in hd_list]
- elif align in ["left", "top"]:
+ elif align in ["left", "bottom"]:
descent = 0.
offsets = [d for h, d in hd_list]
- elif align in ["right", "bottom"]:
+ elif align in ["right", "top"]:
descent = 0.
offsets = [height - h + d for h, d in hd_list]
elif align == "center": |
Bug summary
For the
align
parameter inHPacker
, the optionstop
andbottom
seems reversedCode for reproduction
Actual outcome
Expected outcome
Additional information
No response
Operating system
No response
Matplotlib Version
3.6.2
Matplotlib Backend
No response
Python version
No response
Jupyter version
No response
Installation
No response
The text was updated successfully, but these errors were encountered: