Skip to content

FIX: make sure all ticks show up for colorbar minor tick #12099

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 15, 2018
Merged

FIX: make sure all ticks show up for colorbar minor tick #12099

merged 2 commits into from
Sep 15, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Sep 11, 2018 •
edited
Loading

PR Summary

#12095 shows that if you expect a minor colorbar tick right at the end of a colorbar it sometimes doesn't show up due to floating point precision issues. This fixes that...

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • n/a New features are documented, with examples if plot related
  • n/a Documentation is sphinx and numpydoc compliant
  • n/a Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • n/a Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak mentioned this pull request Sep 11, 2018
Closed
@jklymak
Copy link
Member Author

jklymak commented Sep 11, 2018

Cross-ref #11004

@jklymak jklymak added this to the v3.0 milestone Sep 11, 2018
@jklymak
Copy link
Member Author

jklymak commented Sep 11, 2018

Milestonig for backport, though I don't quite agree that its "Release Critical" 😉

timhoffm
timhoffm approved these changes Sep 12, 2018
View reviewed changes
lib/matplotlib/colorbar.py
@@ -267,7 +267,8 @@ def __call__(self):
vmin = self._colorbar.norm.vmin
vmax = self._colorbar.norm.vmax
ticks = ticker.AutoMinorLocator.__call__(self)
return ticks[(ticks >= vmin) & (ticks <= vmax)]
rtol = (vmax - vmin) * 1e-10
return ticks[(ticks >= vmin - rtol) & (ticks <= vmax + rtol)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a thought: Can it happen, that vmin > vmax? If so, neither the original nor the fixed code would work in that case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but that gets checked and swapped inside the method before this code

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that vmax and vmin get flipped if they are specified out of order. Which maybe is bad behaviour? But its what colorbar does...

@jklymak
Copy link
Member Author

jklymak commented Sep 12, 2018

marking WIP:

  1. needs tests
  2. I guess I think MaxNLocator should be doing this itself; i.e. it should not (by default) return ticks that are out of range. Thats an API change though, and may break other things, so maybe beyond this PR...

@jklymak
Copy link
Member Author

jklymak commented Sep 12, 2018

This is fine for now. New test fails master, passes with this PR. Yes, I could parameterize the test, but I don't think its so unwieldly as is.

The more general issue of MaxNLocator can get its own PR where it can be properly debated.

@jklymak
Copy link
Member Author

jklymak commented Sep 12, 2018

Doc build seems broken, but I don't think its this PR. Something w/ stix fonts...

@tacaswell tacaswell modified the milestones: v3.0, v3.1 Sep 15, 2018
@tacaswell tacaswell merged commit 2c77256 into matplotlib:master Sep 15, 2018
@lumberbot-app
Copy link

lumberbot-app bot commented Sep 15, 2018

Something went wrong ... Please have a look at my logs.

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Sep 15, 2018
@tacaswell
Merge pull request matplotlib#12099 from jklymak/fix-all-cbar-ticks
53f24c2 
FIX: make sure all ticks show up for colorbar minor tick
@tacaswell tacaswell mentioned this pull request Sep 15, 2018
Merged
@Carreau
Copy link
Contributor

Carreau commented Sep 15, 2018

Something went wrong ... Please have a look at my logs.

When things like that happen, feel free to ping me. This was an actual bug that (should) be fixed.

@tacaswell
Copy link
Member

@Carreau I would have gotten there eventually....Trying to get 3.0 finished off today 🤞

Could you also add a link to the logs in that error message? The question I was going to ask you first was "remind me how to find the logs...."

@pharshalp
Copy link
Contributor

I should have noticed this before but I forgot to add a reference to the newly introduced (in PR #11584 for 3.0) cbar.minorticks_on/off methods in the examples section (created a quick PR #12130 for that just now).

@Carreau
Copy link
Contributor

Carreau commented Sep 15, 2018

Could you also add a link to the logs in that error message? The question I was going to ask you first was "remind me how to find the logs...."

I can try, but not directly, as the link changes regularly because of authentication:

https://dashboard.heroku.com/apps/meeseeksbot

Is the closest I can get for you, then click on papertrail.

The bug is (was?) in an optimisation I was doing of reusing previously cloned matplotlib, where tring to fetch a branch fails as it is already the checked-out branch.

That was becoming necessary as matplotlib takes ~70 seconds to clone which is too close the timeout where the bot get killed.

peterkinalex
peterkinalex reviewed Sep 15, 2018
View reviewed changes
Copy link

@peterkinalex peterkinalex left a 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 :

@matplotlib matplotlib deleted a comment from peterkinalex Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterkinalex peterkinalex peterkinalex left review comments

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: ticks axis labels
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jklymak @Carreau @tacaswell @pharshalp @peterkinalex @timhoffm