Skip to content

FIX: colorbar minorticks when rcParams['x/ytick.minor.visible'] = True #16469

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 1 commit into from
Feb 11, 2020
Merged

FIX: colorbar minorticks when rcParams['x/ytick.minor.visible'] = True #16469

merged 1 commit into from
Feb 11, 2020

Conversation

pharshalp
Copy link
Contributor

PR Summary

closes #16468

PR Checklist

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

jklymak
jklymak reviewed Feb 11, 2020
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Can this just be one line?

@pharshalp
Copy link
Contributor Author

pharshalp commented Feb 11, 2020 •
edited
Loading

Realized that this needs a test as soon as submitted the PR... can't change it back to a draft PR :(
will update with a test soon. UPDATE: added a test.

@pharshalp
Copy link
Contributor Author

pharshalp commented Feb 11, 2020 •
edited
Loading

Can this just be one line?

gh isn't showing me the line you are trying to mention. Could you please elaborate? Thanks!

@jklymak
Copy link
Member

jklymak commented Feb 11, 2020 •
edited
Loading

Just call self.minorticks_on(rcpram[minirticks]) ?

EDIT: actually I guess that doesn't work because long_axis isn't defined yet.

@jklymak
Copy link
Member

jklymak commented Feb 11, 2020

I guess I'm not sure we should be turning the minorticks on at all based on the 'ytick.minor.visible' value? I am rather shocked that we have no rcParams for colorbars, but maybe we need one here?

@pharshalp
Copy link
Contributor Author

pharshalp commented Feb 11, 2020 •
edited
Loading

you are right about the one liner. pushed a change.. and yes I agree that we need dedicated rcParams for colorbar. I am just trying to fix the existing incorrect behavior.

jklymak
jklymak approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot @pharshalp

@anntzer anntzer added this to the v3.3.0 milestone Feb 11, 2020
anntzer
anntzer approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

thanks for the pr.
anyone can merge postci.

@pharshalp
fix colorbar minorticks when rcParams['x/ytick.minor.visible'] = True
cef2d95 
Added a test

Replaced the unnecessary logic with a one liner as suggested

Better docs for test

Removed random data from test

flake8 fix
@pharshalp
Copy link
Contributor Author

pharshalp commented Feb 11, 2020 •
edited
Loading

addressed flake8 complaints (squashed and force pushed). CI green

@jklymak jklymak merged commit 79fba06 into matplotlib:master Feb 11, 2020
@pharshalp pharshalp deleted the cbar_minorticks branch March 4, 2020 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anntzer anntzer anntzer approved these changes

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
topic: color/colorbar
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

incorrect cbar minor ticks for extend regions when x/ytick.minor.visible is True
3 participants
@pharshalp @jklymak @anntzer