| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for the contribution!
There are two things to watch out for:
A hard change form %-style to to new-style for the the format parameter would be a breaking change. We cannot do that because users would have to stictly adapt their code with the changed Matplotlib version, which would be really annoying.
What you have to do is accept new style format strings along-side %-style.
Mapping formatter by a property is a separate topic. I suggest to follow up on this in a separate PR if you are interested. Deduplicating state by using a property instead of holding the formatter in self.formatter and in the long axis seems a reasonable approach, however there are some things to watch out for:
You'll loose the formatter with your approach if you switch the orientation. I'm not quite sure if switching the orientation works at all, but if so that's a de-facto feature that we have to continue to support.
As implemented here, this would allow for code like
>>> cbar.formatter = "{x:.3f}"
>>> cbar.formatter
<StrMethodFormatter>
This asymmetry is a somewhat unexpected behavior for a property, and I'm not sure, we want to allow that.
The same mechanism is used for locator and minorlocator. If we switch to properties all three should be changed together.
Sorry, something went wrong.
|
The long-term goal is to make colorbar axises as similar as possible to normal axises. Ideally colorbar.locator and colorbar.formatter would just be dumb reflections of what is on the long axis. |
Sorry, something went wrong.
|
@timhoffm thanks for the review! Yes, I agree that the way it is done here is unaffordable breaking change.
So, the idea is to revert the changes and do something like if isinstance(format, str):
# Check format type between FormatStrFormatter and StrMethodFormatter
try:
self.formatter = ticker.FormatStrFormatter(format)
_ = self.formatter(0)
except TypeError:
self.formatter = ticker.StrMethodFormatter(format)
else:
self.formatter = format # Assume it is a Formatter or None?
Yes, I can send other PRs with that however I do not quite understand the whole picture of what it can break etc (some guidance could be needed).
Could you please explain what it the switch of the orientation (with some code if possible) ? Thanks
Yes, Thomas said something like that during the mentored sprint and that's why I went using property. |
Sorry, something went wrong.
Yes.
I thought you could swich the orienation by assigning to Colorbar.orientation from matplotlib.ticker import PercentFormatter plt.imshow(np.random.random((3, 3))) cb = plt.colorbar(format=PercentFormatter(), orientation='vertial') cb.orientation = 'horizontal' but apparently, this does not have any effect.
Currently, Colorbar holds the state and pushes the information down to the respective axis when needed; e.g. matplotlib/lib/matplotlib/colorbar.py Line 563 in b525983 This is safe against operations like changing orientation (i.e. long and short axis) or clearing of an axis (I don't know that is happens, but would be ok in the current scenario). If the axis holds the state instead, we have to make sure that it's really kept consistent, which could get messed up if long and short axis are exchanged. |
Sorry, something went wrong.
|
First, I think changing the orientation of a colorbar after creating it is a non-starter, and certainly not something we have done previously. cb.orientation should probably be read-only somehow, but... For this PR, we have two separate issues that we are looking at:
Given that these are pretty orthogonal, I agree with @timhoffm that this should maybe be made into two PRs, with 1) being a lot easier than 2)! |
Sorry, something went wrong.
both StrMethodFormatter and FormatStrFormatter if format is str
|
@timhoffm @jklymak thanks for the details and the info. I reverted the code and added as proposed in a previous message a way to switch between 2 formatters if input format is str. Updated also the tests. Please let me know if this is sufficient for the PR "1". Thanks! |
Sorry, something went wrong.
There was a problem hiding this comment.
Module fixing flake8.
Sorry, something went wrong.
|
Thanks @vfdev-5 ! And thanks for participating in the sprint! |
Sorry, something went wrong.
|
@jklymak I'd like to provide a follow-up PR with setter/getter property for cb.format and maybe other attributes. Should we discuss the scope somewhere in a new issue or I can send a draft PR and we can affine it there ? |
Sorry, something went wrong.
|
I usually prefer a draft PR, even if its just a straw man for discussion, so long as it is not too much work |
Sorry, something went wrong.
|
This should still get a what's new entry. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Fixes #21378
PR Summary
This PR introduces formatter as property to fetch axis format and thus new style string formatter is used
If applying a similar logic to major and minor locators, we can end up with removing the code from update_ticks.
Tricky part is with self.norm.
If we are ok with this solution, I can work on doing the same for locator and minorlocator.
Context: PR from PyData Global 2021 mentored sprint
cc @tacaswell
PR Checklist