Skip to content

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

Merged
tacaswell merged 2 commits into
matplotlib:masterfrom
jklymak:fix-all-cbar-ticks
Sep 15, 2018
Merged

FIX: make sure all ticks show up for colorbar minor tick#12099
tacaswell merged 2 commits into
matplotlib:masterfrom
jklymak:fix-all-cbar-ticks

Conversation

@jklymak

@jklymak jklymak commented Sep 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

jklymak commented Sep 11, 2018

Copy link
Copy Markdown
Member Author

Cross-ref #11004

@jklymak jklymak added this to the v3.0 milestone Sep 11, 2018
@jklymak

jklymak commented Sep 11, 2018

Copy link
Copy Markdown
Member Author

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

timhoffm
timhoffm approved these changes Sep 12, 2018
View reviewed changes
Comment thread lib/matplotlib/colorbar.py
ticks = ticker.AutoMinorLocator.__call__(self)
return ticks[(ticks >= vmin) & (ticks <= vmax)]
rtol = (vmax - vmin) * 1e-10
return ticks[(ticks >= vmin - rtol) & (ticks <= vmax + rtol)]

@timhoffm timhoffm Sep 12, 2018

Copy link
Copy Markdown
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.

@jklymak jklymak Sep 12, 2018

Copy link
Copy Markdown
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

@jklymak jklymak Sep 12, 2018

Copy link
Copy Markdown
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

jklymak commented Sep 12, 2018

Copy link
Copy Markdown
Member Author

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

jklymak commented Sep 12, 2018

Copy link
Copy Markdown
Member Author

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

jklymak commented Sep 12, 2018

Copy link
Copy Markdown
Member Author

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

lumberbot-app Bot commented Sep 15, 2018

Copy link
Copy Markdown

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

Carreau commented Sep 15, 2018

Copy link
Copy Markdown
Contributor

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

tacaswell commented Sep 15, 2018

Copy link
Copy Markdown
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

pharshalp commented Sep 15, 2018

Copy link
Copy Markdown
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

Carreau commented Sep 15, 2018

Copy link
Copy Markdown
Contributor

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

@peterkinalex peterkinalex left a comment

Copy link
Copy Markdown

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

@timhoffm timhoffm timhoffm approved these changes

+1 more reviewer

@peterkinalex peterkinalex peterkinalex left review comments

Reviewers whose approvals may not affect merge requirements

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