Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix ngrids support in axes_grid.Grid().#27972
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
timhoffm commentedMar 24, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Since no user has reported that this is broken for more than 3 years, I’m considering removing that parameter. The naming is confusing and the bug can be regarded a very hard deprecation 😏 . If we want such functionality, we could either re-introduce it under a better name such as |
I don't really mind either way. However, if you're going to refactor this, I wonder if it would be better to instead have a I guess the required signature would be defarrange_grid(axes,# array or dictaxes_pad=0.02,aspect=???,label_mode="L",# not even clear this is needed; label_outer() could be enoughcbar_mode="each"/"single"/"edge"/"none",# "none" case covers the plain Grid()# followed with the cbar_foo kwargs): ... |
How is that different than layout='compressed'? |
I actually agree with the comment in#27963 that we should just get rid of axes_grid and promote layout="compressed" as much as possible. (I guess the "different-sized images" case could perhaps be handled nowadays via box_aspect? Does CL know anything about that?) |
CL knows about width- and height ratios. However it doesn't know about the aspect ratio of the axes inside, and is best suited to axes that don't have aspect constraints. (You can write the problem including aspect constraints but the optimization goes from 2n to n^2 and becomes very slow.) I'd not be against adding a new method to the top level library that does a grid of fixed aspect axes like this. But we should carefully decide how it is different than what we already have and if it's worth it, versus letting the folks who need this do a bit of fussing with width and height ratios. |
(I don't have strong feelings about axes_grid; this PR was mostly motivated by fixing a regression I caused some time ago...) |
I still want to decide whether we use the fact that this parameter was not working for a long time to remove or rename it. Since this was found internally and not by users, we have no indication that somebody is using it, so no need to hurry and fix it. |
ebarcelosf commentedOct 16, 2024
hi, can i solve this issue? |
timhoffm commentedOct 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This is a solution proposal that needs decision from core developers. You cannot help here. |
@timhoffm Can we reach a decision on this? I still mildly prefer restoring support for ngrids (perhaps prior to larger changes later) but don't really mind if we decide instead to just drop it. But the current situation (a non-working but documented kwarg) just seems the worst. |
timhoffm commentedFeb 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm still for deprecate and note that this has not been working anyway. Nobody has complained about this the last 4 years. If you think the feature is helpful let's reintroduce it right away under a new meaningful name such as Edit: Or fix and |
OK, renamed. |
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.
Please fix the Attributes doc. Read-only property is optional.
Uh oh!
There was an error while loading.Please reload this page.
Syntax seems broken somewhere. |
oopsie. refixed. |
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.
LGTM - I think worth notingImageGrid
has changed too. Feel free to self merge if the API note is updated
This attribute has been deprecated and renamed ``n_axes``, consistently with | ||
the new name of the `~.axes_grid.Grid` constructor parameter that allows | ||
setting the actual number of axes in the grid (the old parameter, ``ngrids``, | ||
did not actually work since Matplotlib 3.3). |
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.
did not actually work since Matplotlib 3.3). | |
did not actually work since Matplotlib 3.3). | |
The same change has been made in ``axes_grid.ImageGrid``. |
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.
done
This also restores axes_row and axes_column to their behavior prior to88e0d84(which is the commit that broke ngrids support).
b32de5c
intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This also restores axes_row and axes_column to their behavior prior to88e0d84 (which is the commit that broke ngrids support).
Closes a small part of#27963.
PR summary
PR checklist