I'll assert, without proof, that passing a single string, a mosaic list of strings, or a mosaic list all of the same type is more common than passing arbitrary unrelated hashables. Thus it is somewhat convenient if the return type stipulates that the resulting dictionary is also keyed with strings or the common type.

This also fixes the type of theper_subplot_kw argument, which also allows dictionary keys of tuples of the entries.

I don't like that we need a few ignores, but this does work from an external point of view:

from typing import TYPE_CHECKINGimport matplotlib.pyplot as pltif not TYPE_CHECKING:    reveal_type = printfig0, ax0 = plt.subplot_mosaic('ab;cd')reveal_type(ax0)  # Should be str keysfig1, ax1 = plt.subplot_mosaic([['a', 'b'], ['c', 'd']])reveal_type(ax1)  # Should be str keysfig2, ax2 = plt.subplot_mosaic([[0, 1], [2, 3]])reveal_type(ax2)  # Should be int keysfig3, ax3 = plt.subplot_mosaic([['a', 'b'], ['c', 'd'], [0, 1]])reveal_type(ax3)  # Should be any Hashable keysfig = plt.figure()ax4 = fig.subplot_mosaic('ab;cd')reveal_type(ax4)  # Should be str keysax5 = fig.subplot_mosaic([['a', 'b'], ['c', 'd']])reveal_type(ax5)  # Should be str keysax6 = fig.subplot_mosaic([[0, 1], [2, 3]])reveal_type(ax6)  # Should be int keysax7 = fig.subplot_mosaic([['a', 'b'], ['c', 'd'], [0, 1]])reveal_type(ax7)  # Should be any Hashable keys

produces:

foo.py:9: note: Revealed type is "builtins.dict[builtins.str, matplotlib.axes._axes.Axes]"foo.py:12: note: Revealed type is "builtins.dict[builtins.str, matplotlib.axes._axes.Axes]"foo.py:15: note: Revealed type is "builtins.dict[builtins.int, matplotlib.axes._axes.Axes]"foo.py:18: note: Revealed type is "builtins.dict[typing.Hashable, matplotlib.axes._axes.Axes]"foo.py:23: note: Revealed type is "builtins.dict[builtins.str, matplotlib.axes._axes.Axes]"foo.py:26: note: Revealed type is "builtins.dict[builtins.str, matplotlib.axes._axes.Axes]"foo.py:29: note: Revealed type is "builtins.dict[builtins.int, matplotlib.axes._axes.Axes]"foo.py:32: note: Revealed type is "builtins.dict[typing.Hashable, matplotlib.axes._axes.Axes]"Success: no issues found in 1 source file

i.e., for any common-types inputs, the dictionary uses that type as key, whereas mixed types is a genericHashable.

PR checklist

[n/a] "closes #0000" is in the body of the PR description tolink the related issue
new and changed code istested
[n/a]Plotting related features are demonstrated in anexample
[n/a]New Features andAPI Changes are noted with adirective and release note
[n/a] Documentation complies withgeneral anddocstring guidelines

QuLogic added the topic: typing label

Aug 10, 2023

QuLogic added this to thev3.8.0 milestone

Aug 10, 2023

Copy link

MemberAuthor

QuLogic commentedAug 10, 2023

Looking overmypy results, this introduces a few errors fortest_figure.py, because it now requires 2D lists and some tests check that 1D lists will error (so this is expected.)

It also shows that a mosaic with a nested 2D list can't catch the overload, even if all contained items arestr. This is because mixingstr withlist[list[str]] as list element makes mypy infer that it is overall alist[object]. It's unfortunate that we can't catch this case, but the problem is on the caller side.

I think if callers want to be explicit, they could annotate their mosaic aslist[HashableList[str]]. It might be a bit annoying to have list appear twice there; maybe we should have aMosaicList = list[HashableList] sort of alias?

Copy link

MemberAuthor

QuLogic commentedAug 10, 2023•
edited
Loading

I also checked over the galleries, and it fixes a few issues like:

fig,axd=plt.subplot_mosaic(...)fortitle,axinaxd.items():ax.set_title(title)# Previously failed as set_title didn't take a Hashable.

However, I see two issues:

If you save the mosaic to a variable, it appears to becomelist[list[str]], and IIUC,list is invariant and doesn't matchlist[Hashable[list]] any more (whereas the literal was allowed a bit more leeway).
One example uses a 2D NumPy array instead of a list of lists.

I'm not sure if we should be using aSequence instead oflist as the basis ofHashableList.

QuLogic force-pushed thestr-mosaic branch from6fcee04 to757bb2fCompare

August 10, 2023 23:14

Copy link

Member

ksunden commentedAug 16, 2023

This is failing the doc build because of the changes toHashableList

QuLogic force-pushed thestr-mosaic branch from757bb2f tob4d9805Compare

August 17, 2023 08:49

Copy link

Member

ksunden commentedAug 17, 2023

Well it looks like you sorted the Hashable list sphinx problem, but sphinx introduced new problems,PR Submitted upstream

QuLogic force-pushed thestr-mosaic branch fromb4d9805 to4530267Compare

August 19, 2023 00:44

ksunden reviewed

Aug 31, 2023

View reviewed changes

lib/matplotlib/pyplot.py

		**fig_kw: Any
		) -> tuple[Figure, dict[str, matplotlib.axes.Axes]] \| \
		tuple[Figure, dict[_T, matplotlib.axes.Axes]] \| \
		tuple[Figure, dict[Hashable, matplotlib.axes.Axes]]:

Copy link

Member

ksundenAug 31, 2023

Choose a reason for hiding this comment

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

Type hints can be removed from the implementation signature with overloads

they just clutter everything up trying to encompass all possible cases, and don't actually add anything that is not covered by the extant type hints

The examples fromPEP 484 and thetyping module docs omit the type hints on the actual implementation.

Copy link

Member

ksundenAug 31, 2023

Choose a reason for hiding this comment

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

(Thismay allow getting rid of the type ignores? not sure)

Copy link

MemberAuthor

QuLogicAug 31, 2023

Choose a reason for hiding this comment

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

If I drop all the types on the function, thenmypy --strict lib/matplotlib gives:

lib/matplotlib/pyplot.py:1652: error: Function is missing a type annotation  [no-untyped-def]

and all the type ignores are no longer needed, but that's because the function's untyped, no? It is fine without--strict though.

Copy link

Member

ksundenAug 31, 2023

Choose a reason for hiding this comment

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

That is annoying, I don't wholly agree with that, looks like the reasoning is that it won't type check the implementation then, but I'm nottoo concerned about that...

I could go either way.

Copy link

Member

ksundenSep 4, 2023

Choose a reason for hiding this comment

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

I think it can be improved by removing theHashable path (and making the type var havebound=Hashable in there... may actually be able to get rid ofstr from all but the mosaic arg...

Actually its not clear to me whatHashableList[Hashable] is doing for us at all, shouldn't that just be the same asHashableList, just not having the typevar bound to any particular type?

github-actionsbot added the status: needs rebase label

Sep 2, 2023

TYP: Add common type overloads of subplot_mosaic

58d5aad

I'll assert, without proof, that passing a single string, a mosaic listof strings, or a mosaic list all of the same type is more common thanpassing arbitrary unrelated hashables. Thus it is somewhat convenient ifthe return type stipulates that the resulting dictionary is also keyedwith strings or the common type.This also fixes the type of the `per_subplot_kw` argument, which alsoallows dictionary keys of tuples of the entries.

QuLogic force-pushed thestr-mosaic branch from4530267 to58d5aadCompare

September 6, 2023 20:11

ksunden approved these changes

Sep 6, 2023

View reviewed changes

github-actionsbot removed the status: needs rebase label

Sep 6, 2023

greglucas approved these changes

Sep 7, 2023

View reviewed changes

greglucas merged commitb719012 intomatplotlib:main

Sep 7, 2023

Copy link

lumberbot-appbot commentedSep 7, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout v3.8.xgit pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 b71901283457c9cb3cc0a6fc0261a2bba729c823

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #26491: TYP: Add common-type overloads of subplot_mosaic'

Push to a named branch:

git push YOURFORK v3.8.x:auto-backport-of-pr-26491-on-v3.8.x

Create a PR against branch v3.8.x, I would have named this PR:

"Backport PR#26491 on branch v3.8.x (TYP: Add common-type overloads of subplot_mosaic)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove theStill Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free tosuggest an improvement.

lumberbot-appbot added the Still Needs Manual Backport label

Sep 7, 2023

QuLogic deleted the str-mosaic branch

September 7, 2023 08:21

QuLogic pushed a commit to QuLogic/matplotlib that referenced this pull request

Sep 7, 2023

Backport PRmatplotlib#26491: TYP: Add common-type overloads of subpl…

0421fde

…ot_mosaic

QuLogic mentioned this pull request

Sep 7, 2023

Backport PR #26491 on branch v3.8.x (TYP: Add common-type overloads of subplot_mosaic)#26712

Merged

1 task

QuLogic removed the Still Needs Manual Backport label

Sep 7, 2023

ksunden added a commit that referenced this pull request

Sep 11, 2023

Merge pull request#26712from QuLogic/auto-backport-of-pr-26491-on-v…

d0321ee

…3.8.xBackport PR#26491 on branch v3.8.x (TYP: Add common-type overloads of subplot_mosaic)

ksunden mentioned this pull request

Sep 15, 2023

merge up v3.8.0#26785

Merged

5 tasks

Labels

topic: typing

3 participants

Movatterモバイル変換

Uh oh!

TYP: Add common-type overloads of subplot_mosaic#26491

TYP: Add common-type overloads of subplot_mosaic#26491

Uh oh!

Conversation

QuLogic commentedAug 10, 2023

PR summary

PR checklist

Uh oh!

QuLogic commentedAug 10, 2023

Uh oh!

QuLogic commentedAug 10, 2023• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ksunden commentedAug 16, 2023

Uh oh!

ksunden commentedAug 17, 2023

Uh oh!

ksundenAug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ksundenAug 31, 2023

Choose a reason for hiding this comment

Uh oh!

QuLogicAug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ksundenAug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ksundenSep 4, 2023

Choose a reason for hiding this comment

Uh oh!

lumberbot-appbot commentedSep 7, 2023

Uh oh!

Uh oh!

QuLogic commentedAug 10, 2023•
edited
Loading