-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add Spines class as a container for all Axes spines #17107
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
Conversation
Haven't reviewed carefully, but you may want to compare with the AxisDict approach in mpl_toolkits, which similarly overrides getitem so that one can write |
98d122a
to
c54a2e0
Compare
Comparison with
|
We don't necessarily have to copy the AxesDict approach here. However, I think we're also trying to bring axes_grid towards inclusion in the main lib (#17095), so I guess the converse question could be, would it make sense to change AxesDict to use this machinery (with the proper deprecation period, yada yada)? (You don't have to write the adaptor API for this PR, but it would be better to at least check something similar can be used for axes_grid.) |
Without having written the prove of concept: From what I've seen in the comparison to |
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 don't have any changes to request, but I expect this would have to be checked against Cartopy, WCSAxes, and maybe yt, so blocking just in case.
Checked for possible downstream incompatibilities. Cartopy:
all of these usages are supported by this PR. - I've run the Cartopy tests: the 4 same tests failed with and without this PR. WCSA Axes: yt: |
This seems pretty nice to me. I assume users can add more spines as before. How do we get the list of spines? |
The list of spines is still By subclassing |
Ping @QuLogic #17107 (comment) addresses your change request. |
On thing I had missed is that in #17107 (comment) @timhoffm stated
but that is actually missing an important point about AxesDict which is not provided by SpinesProxy: you can actually also select a subset of Axes using AxesDict, using e.g. I'm not saying we absolutely need this feature, but would at least like to point out that this is something much easier to retrofit when using indexing notation rather than attribute access. |
I agree that we should support subsets, but via Thinking a bit more, I take the standpoint that we should support
This is an API similar to pandas and thus hopefully easy to remember. Differences are: 1) We have |
Do we want |
One of the main reasons for this was to be able to access the spines via attributes ( |
d5e9f0b
to
1682307
Compare
I'm a bit confused whether you do or do not want to keep |
Oh, will fix that. .all is gone. |
Co-authored-by: yech1990 <yech1990@gmail.com>
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.
This is very nice.
You could use the ax.spines[['top', 'bottom']].set_visible(False)
type of construct many more places in the examples, but I certainly wouldn't insist on it. It aids readability and reduces LOC whenever a setting is applied to more than one spine.
This has two positive reviews. IMHO it could be merged, or is there still anything to be discussed? |
PR Summary
Closes #17099. Alternative approach to #17100, as proposed in #17099 (comment).
With this, you can do (updated based on discussion below):
and it should be mostly backward-compatible (with the exception that I do not support OrderedDict-specific methods) for
spines
.Note: This PR consists of two patches for easier review: The first patch introduces the new API, the second patch applies it throughout the library (i.e. you can see the API in action here).
Open issues:
OrderedDict
?spines.top.set_visible
)? - Access by itemspines['top']
will stay anyway for backward-compatibilityspines.all.set_visible(False)
?This is now implemented as
spines[:].set_visible(False)
.MutableMapping
? - I.e. theOrderedDict
methodsspines.popitem()
andspines.move_to_end()
are dropped without deprecation. - Could add deprecation dance for these as well if really needed.spines['left', 'right']
or (personally preferred)spines[['left', 'right']]
?Yes. Implemented.
Mapping
instead of aMutableMapping
.spines.all
throught library and examples where suitable