-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix cl subgridspec #22144
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
Fix cl subgridspec #22144
Conversation
# get a unique representation: | ||
rep = object.__repr__(gs) + 'top' |
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.
Can you not simply use the memory address of the object: id(gs)
?
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.
I'm not sure. gs
is used as a key as well, so I wanted to differentiate.
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.
If the use of the key is to 1) be unique to each gridspec and 2) does not need to be human readable, I would go with using id(gs)
. Either way, I think the keys should be of a similar type, so it would be good to modify the if block above this to use either id(gs)
or object.__repr__(gs)
.
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.
The whole set of code usually just uses layoutgrid[obj]
where obj
is the actual object, be it a gridspec, figure etc. We could make them all be layoutgrid[id(obj)]
but that would introduce a lot of extra typing.
Here, this is just a dummy shell so that the hierarchy of subgridspecs is the same as other objects, in that they all have a parent that can hold suptitles, etc. There is no associated object on the figure corresponding to this, so I just am using a dummy here. The problem with the original code is that the dummy wasn't unique!
We can certainly use id
, but that loses any hint of what the object was in the key. object.__repr__
keeps the type info and the id
. i.e. for this case we get a string "<matplotlib.gridspec.GridSpecFromSubplotSpec object at 0x17b686790>top"
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.
Another thought - since __repr__
is typically a string that can be passed to eval()
to create an identical copy of the object, it isn't always unique, e.g.:
>>> 'abc'.__repr__()
"'abc'"
So I would still say if you want to guarentee uniqueness, use id()
in case we change GridSpecFromSubplotSpec.__repr__
to a non-instance-unique string in the future.
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.
When we use the base class object.__repr__
, this should always result in f"<{gs.__class__.__name__} object at {id(gs)}>"
. Uniqueness should be guaranteed. But maybe just just the explicit formatting instead of the slightly unusual object.__repr__
.
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.
You could perhaps use layoutgrids[(gs, "top")]
(which can even be spelled layoutgrid[gs, "top"]
).
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.
Ah OK, thats reasonable. It really never gets referenced except by the child via child.parent
, so it just needs to be identifiable for debug purposes.
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.
Newest commit just uses the tuple...
…144-on-v3.5.x Backport PR #22144 on branch v3.5.x (Fix cl subgridspec)
PR Summary
Closes #22143
If subgridspecs were identical, then their strings were identical, and the dictionary we had for the layoutgrids was being addressed with the same key twice. Changed the key to a unique repr for the subgridspec.
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).