Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
ENH: fix colorbar bad minor ticks#11584
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@pharshalp Thanks for the PR :)! If you have not already seen it, there are a few informations about writing testshere in the documentation. FWIW, the current trend in Matplotlib is to avoid “image tests” as much as possible (mostly to keep the size of git history reasonable). Here, one could for example “simply” test that the tick values are the expected ones, rather than comparing to a fully pre-rendered reference picture. (Such a test could be added to |
lib/matplotlib/colorbar.py Outdated
| locs = locs.compress(cond) | ||
| return self.raise_if_exceeds(np.array(locs)) | ||
| class _ColorbarLogLocator(ticker.LogLocator): |
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 CI will likely complain if there are not2 blank lines before a new class definition.
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.
Thanks! just pushed a fix.
This looks like a great start. Will definitely need a test! Have a look in |
added a test that checks for the correct behavior of cbar.minorticks_on and cbar.minorticks_off(). |
lib/matplotlib/colorbar.py Outdated
| self.vmax = self._colorbar.norm.vmax | ||
| self.ndivs = n | ||
| def __call__(self): |
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't you just callticker.AutoMinorLocator and then trim the values?
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.
ticker.AutoMinorLocator doesn't seem to have a tick_values method that would return the tick locations.
deftick_values(self,vmin,vmax):raiseNotImplementedError('Cannot get tick locations for a ''%s type.'%type(self))
Instead, AutoMinorLocatior has a method calledcall which returns the tick values... (the calculation for the locations of the minor ticks is done insidecall which I have copied and modified).
def__call__(self):'Return the locations of the ticks'
Please let me know if I am missing a better way of doing this. Thanks!
ahh I see what you meant there... will fix it.
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.
Thanks@jklymak I have pushed a cleaner version now.
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 looks almost done, and I think the code is all correct. Nice test.
I think it needs a "whats new" entry because I don't think colorbars hadminorticks_on/off methods before. I think its a reasonable addition, though I guess the "old" way to do this would have been to simply act on the colorbar's axes directly.
Please add an entry todoc/users/next_whats_new following one of the templates there (don't forget togit add the new file before committing)!
Thanks again, this is very nice...
Thank you@jklymak for your prompt help in reviewing this. I have added a 'whats new' entry. Since technically this is not a new feature (we are just fixing an incorrect behavior), I am not sure if the "New features are documented, with examples if plot related" rule applies here. |
But the method to add minor ticks to colorbar is new, isn’t it? Yes squashing is preferred. Thanks |
lib/matplotlib/colorbar.py Outdated
| else: | ||
| long_axis = ax.xaxis | ||
| long_axis.set_minor_locator(_ColorbarAutoMinorLocator(self)) |
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.
Sorry to review in bits and drabs...
What happens if the colorbar is logarithmic? Is this function still a good idea? Or should you be setting a_ColorbarLogMinorLocator?
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 are right... I completely overlooked that case. Will fix it.
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.
hmm... this is an interesting journey! From what I understand, LogLocator only returns Major ticks and AutoMinorLocator doesn't work with log scale... I guess I will have to dig deeper.
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 think the log locator looks after the minor ticks, but I could be misremembering. So in this case, you probably just need to be careful that "minorticks_on" just doesn't do anything so as not to step on the log locator's toes.
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.
made sure that minorticks_on and minorticks_off doesn't do anything when called on a logarithmic colorbar axis (also added a warning letting the user know about this).
pharshalp commentedJul 7, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Modified an existing example to show off the utility of cbar.minorticks_on() https://matplotlib.org/gallery/color/colorbar_basics.html#sphx-glr-gallery-color-colorbar-basics-py
How can I check that the "Documentation is sphinx and numpydoc compliant"?. I do not have any experience with sphinx, numpydoc. |
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.
Not blocking on my gripes on the example. Its functional, just not very good to look at. Otherwise, I think this is very helpful and well done. Thanks!
Thank you@jklymak for your guidance and for reviewing this. Thanks to@afvincent too! |
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.
Thanks. A well written PR. Only some minor comments.
| Add ``minorticks_on()/off()`` methods for colorbar | ||
| -------------------------------------------------- | ||
| A new method :meth:`~matplotlib.colorbar.Colobar.minorticks_on` is |
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 use`.Colorbar.minorticks_on`
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.
thanks... I wasn't really familiar with this style of documentation so I was just following what I saw in some of the other whats new files :D
| doesn't allow the minor ticks to extend into the regions beyond vmin and vmax | ||
| when the extend `kwarg` (used while creating the colorbar) is set to 'both', | ||
| 'max' or 'min'. | ||
| A complementary method :meth:`~matplotlib.colorbar.Colobar.minorticks_off` |
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.
`.Colorbar.minorticks_off`
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.
done!
lib/matplotlib/colorbar.py Outdated
| This ticker needs to know the *colorbar* so that it can access | ||
| its *vmin* and *vmax*. | ||
| """ | ||
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.
no need for empty line after docstrings
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.
✔️
lib/matplotlib/colorbar.py Outdated
| if self.orientation == 'vertical': | ||
| long_axis = ax.yaxis | ||
| else: | ||
| long_axis = ax.xaxis |
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.
more pythonic:
long_axis = ax.yaxis if self.orientation == 'vertical' else ax.xaxisThere 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.
✔️
lib/matplotlib/colorbar.py Outdated
| """ | ||
| try: | ||
| ax = self.ax | ||
| except AttributeError: |
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 that happen? The attribute is defined inColorbarBase.__init__.
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 are right... removed the try except block
| @@ -270,6 +270,31 @@ def test_colorbar_ticks(): | |||
| assert len(cbar.ax.xaxis.get_ticklocs()) == len(clevs) | |||
| def test_colorbar_minorticks_on_off(): | |||
| # test for #11584 | |||
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.
# test for github issue #11584
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.
✔️
@timhoffm Thanks! addressed your suggestions. |
Added a missing blank line before class definitionadded a test for minorticks_on/offRenamed the test for colorbar minorticks_on/off and added helpful commentCleaned up the __call__ method for _ColorbarAutoMinorLocator as per the suggestion of@jklymakAdded whats new entrymaking sure that minorticks_on/off doesn't act on a logarithmic colorbar axisAdded _ColorbarAutoMinorLocator, minorticks_on/off, test_colorbar_minorticks_on_off and whats new entryAdded an example showing off the utility of cbar.minorticks_on()Cleaning up
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.
Thanks! Anyone can merge after CI pass.
pharshalp commentedJul 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I hope this doesn't come across as being pushy but, I was wondering if this will make it to 3.0 milestone (or if the 3.0 milestone is reserved only for critical fixes). |
If its in master when 3.0 is branched (which hasn't happened yet so far as I know), then it'll be in 3.0.. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
This follows the suggestion given in#11556 and builds a method for colorbar to correctly display minor ticks (without extending into the region beyond vmin, vmax when the extend option is used).
Earlier, the only way to display the minor ticks was to use ax.minorticks_on/off methods
(which didn't respect vmin, vmax values).
With this PR, the colorbar gets its own minorticks_on/off methods.
Fixes#11510 and is a follow up to#11556 .
@jklymak Could you please review this?
This builds on top of your suggestions in the discussion here#11556,#11510.
This is my first (relatively significant) contribution to matplotlib (apart from a minor documentation entry) so I would appreciate any help in how to properly document API change (or if I am missing anything else).
I am marking this as WIP since I am not sure how to add tests and other entries from the PR checklist (I will try to understand the requirements for each item in the list and will work on those soon).PR Checklist
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible wayCorrect behavior (after the PR)
Incorrect behavior