MNT: colorbar locators properties #22244
Conversation
2d1c72c to
b65ec3c
Compare
|
Q: Should we also have |
lib/matplotlib/colorbar.py
Outdated
| """ | ||
| Major `~.Locator` being used for colorbar. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Major `~.Locator` being used for colorbar. | |
| """ | |
| """The major tick `.Locator` of the colorbar.""" |
- If there is just one line, we put the quotes on the same line https://matplotlib.org/devdocs/devel/documenting_mpl.html#quote-positions
~removes leading package/module names from links, but here you don't have any, so we can leave it out, which makes the code a bit more readable.- "being used" does not add information -> can be left out.
Similar for all getters. - I've added "tick Locator", while locators are a technical term and you can find out through the link, it may be worth associating all these properties with ticks explicitly.
There was a problem hiding this comment.
Went for Major tick .Locator for the colorbar
IMHO we should not encourage I'm not quite sure whether we need the suggested |
I don't think there is a problem with that. However, I worry that for a normal axes
I don't know that it would make sense to go backwards on this. The point of the recent work is so that colorbar axes do not have their own incompatible ticking system. OTOH I am sympathetic to the idea that changing the location of the colorbar should not |
I consider I was somehow thinking of Note that |
|
Right @greglucas even wanted to make a colorbar be a subclass of Axes, and it wasn't a ton of work. As a medium solution, I'm more than fine w/ I think that is a little orthogonal to this (we need to maintain the locator/formatter properties anyways as they currently exist), but happy to put in the same PR if that is easier. |
That's not the right way, but discussed elsewhere. For now, let's just have a mental model of the similarity and define analogous methods where appropriate.
👍
Yes, we need the properties for compatibility. Everything else can be separate PR. We got a bit side-tracked by your question on the methods. Note that there are still two comments open above. |
410ad06 to
f3bba62
Compare
f3bba62 to
13b4167
Compare
13b4167 to
29ef83e
Compare
There was a problem hiding this comment.
Seems reasonable to me. I just had one small comment on the new _minorformatter private variable.
Some comments for potential follow-up PRs, but shouldn't impact this one.
- This had me confused why we even need to keep track of the
Nones at all. I'm wondering if we can just dispatch the property out to the long-axis get/set methods without needing to keep track of the private property. I think the None is only needed to set the default cases, so I'm wondering if that can be handled elsewhere... - @timhoffm I like the idea of making a
cb.axisproperty, that does seem nice and clean to me for colorbars. One additional consideration would be whether it would be useful to treat this as a "spines"-like API. So you could doax.axis["x"],ax.axis["r"],ax.axis["all"], ... (thinking more generally than just colorbar) - One slight API mismatch here is that we have "locator"/"formatter" on Colorbars, without the "major" qualifier, but the underlying setters/getters use
set_major_locatorinstead ofset_locator. So, something to keep in mind here is that I think Colorbar is the only place without the major qualifier compared to the rest of the library.
| self._locator = None | ||
| self._minorlocator = None | ||
| self._formatter = None | ||
| self._minorformatter = None |
There was a problem hiding this comment.
The None here doesn't get used anywhere below to update a default? It also doesn't get reset in the _reset_locator_formatter_scale() call, so I'm wondering if we need the _minorformatter private variable, or if you can simply call the the get/set methods on the long-axis from the new property.
There was a problem hiding this comment.
I haven't traced minor formatter all the way through the piping yet, however, I added it to be parallel with the other properties. I get a little nervous about how often the colorbar changes state, particularly now that it seems to change state if the mappable's norm is changed ;-) If I change the norm on the mappable, but I have manually set locator, should that stay or should we overwrite it? I think now it stays unless self._locator is None, in which case we use the default appropriate to the norm. We could simplify our lives by saying that any norm changes invalidates any manual setting of the colorbar locators/formatters, and that the user would have to redo those. I think that is what happens if a user changes the scale of an axis, so it would be consistent here.
There was a problem hiding this comment.
I agree that all of this is quite confusing with when things are validated/invalidated in the Colorbar pipeline. What I was trying to get at here was only for the minorformatter. I think you should also set it to None down below too where the other locators/formatters are also reset, since you are now keeping track of it.
@greglucas Thanks for the suggustion! I haven't thought about that possibility before. It's always good to get fresh ideas. However, from a practical point of view I have some doubts:
|
Yes, I would suggest we discourage the properties, and encourage |
| self.formatter = None | ||
| self._locator = None | ||
| self._minorlocator = None | ||
| self._formatter = None |
There was a problem hiding this comment.
| self._formatter = None | |
| self._formatter = None | |
| self._minorformatter = None |
| self._locator = None | ||
| self._minorlocator = None | ||
| 9E88 | self._formatter = None | |
| self._minorformatter = None |
There was a problem hiding this comment.
I agree that all of this is quite confusing with when things are validated/invalidated in the Colorbar pipeline. What I was trying to get at here was only for the minorformatter. I think you should also set it to None down below too where the other locators/formatters are also reset, since you are now keeping track of it.
PR Summary
This is to prepare to address #22233
Currently the user can set the colorbar locator and formatter by direct access to the properties
cb.formatter,cb.locator, andcb.minorlocator. However, they can also setcb.ax.yaxis.set_major_locatoretc.This moves the attributes to properties with setters and getters. The advantage of this is keeping the attributes in sync, but also we can track if the user set the locators via the properties. If not, then we assume we can replace the locators and formatters with defaults.
This does not catch if the user set the locator and formatter using
cb.ax.yaxis.*, so we may still have some things to figure out there.PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand 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).