FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: make sure all ticks show up for colorbar minor tick by jklymak · Pull Request #12099 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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
ConversationCommits (2)ChecksFiles changed
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 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 mentioned this pull request Sep 11, 2018
Closed

jklymak commented Sep 11, 2018

Copy link
Copy Markdown
Member Author

Cross-ref #11004

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

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 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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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 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 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 modified the milestones: v3.0, v3.1 Sep 15, 2018
tacaswell merged commit 2c77256 into matplotlib:master Sep 15, 2018

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
Merge pull request matplotlib#12099 from jklymak/fix-all-cbar-ticks
53f24c2 
FIX: make sure all ticks show up for colorbar minor tick
tacaswell mentioned this pull request Sep 15, 2018
Merged

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 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 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 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 reviewed Sep 15, 2018
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lib / matplotlib / colorbar.py :

matplotlib deleted a comment from peterkinalex Sep 18, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

timhoffm timhoffm approved these changes
+1 more reviewer
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

Back | FazBrowse Home | New Git URL