Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
timhoffm merged 1 commit intomatplotlib:mainfromanntzer:gn
Mar 26, 2025
Merged

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedMar 24, 2024
edited
Loading

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

@timhoffm
Copy link
Member

timhoffm commentedMar 24, 2024
edited
Loading

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 asnaxes, or consider whether there is a better and more flexible API altogether: For examplen_rows_cols could take a 2D bool array, specifying at which positions one wants Axes.

@anntzer
Copy link
ContributorAuthor

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 aarrange_grid() that takes the output of Figure.subplots() or Figure.subplot_mosaic() (i.e. an array or a dict of axes) and sets the locators on them as needed and optionally adds colorbars. This would allow a more orthogonal API (think "composition over inheritance", even though there isn't inheritance here) -- in particular, you wouldn't need provide rect, ngrids, share_all/share_x/share_y (you'd just read these off the axes that are passed to you.

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): ...

@jklymak
Copy link
Member

How is that different than layout='compressed'?

@anntzer
Copy link
ContributorAuthor

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?)

@jklymak
Copy link
Member

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.

anntzer reacted with thumbs up emoji

@anntzer
Copy link
ContributorAuthor

(I don't have strong feelings about axes_grid; this PR was mostly motivated by fixing a regression I caused some time ago...)

@timhoffmtimhoffm added the status: needs comment/discussionneeds consensus on next step labelApr 2, 2024
@timhoffm
Copy link
Member

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.

anntzer reacted with thumbs up emoji

@timhoffmtimhoffm added this to thev3.10.0 milestoneApr 2, 2024
@timhoffmtimhoffm self-assigned thisApr 2, 2024
@ksundenksunden modified the milestones:v3.10.0,v3.11.0Oct 11, 2024
@ebarcelosf
Copy link

hi, can i solve this issue?

@timhoffm
Copy link
Member

timhoffm commentedOct 17, 2024
edited
Loading

This is a solution proposal that needs decision from core developers. You cannot help here.

@anntzer
Copy link
ContributorAuthor

@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
Copy link
Member

timhoffm commentedFeb 21, 2025
edited
Loading

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 asn_axes.

Edit: Or fix and@rename_parameter which is effectively the same. - I just don't want that people are using the unintuitive namengrids.

@anntzer
Copy link
ContributorAuthor

OK, renamed.

Copy link
Member

@timhoffmtimhoffm left a 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.

@QuLogic
Copy link
Member

Syntax seems broken somewhere.

@anntzer
Copy link
ContributorAuthor

oopsie. refixed.

Copy link
Member

@dstansbydstansby left a 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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
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``.

Copy link
ContributorAuthor

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).
@timhoffmtimhoffm merged commitb32de5c intomatplotlib:mainMar 26, 2025
37 of 41 checks passed
@anntzeranntzer deleted the gn branchMarch 26, 2025 23:44
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@timhoffmtimhoffmtimhoffm approved these changes

@dstansbydstansbydstansby approved these changes

Assignees

@timhoffmtimhoffm

Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.

7 participants
@anntzer@timhoffm@jklymak@ebarcelosf@QuLogic@dstansby@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp