Skip to content

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

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
pharshalp:cbar_minorticks
Feb 11, 2020
Merged

FIX: colorbar minorticks when rcParams['x/ytick.minor.visible'] = True#16469
jklymak merged 1 commit into
matplotlib:masterfrom
pharshalp:cbar_minorticks

Conversation

@pharshalp

@pharshalp pharshalp commented Feb 11, 2020

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

@jklymak jklymak left a comment

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.

Can this just be one line?

@pharshalp

pharshalp commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

pharshalp commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Can this just be one line?

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

@jklymak

jklymak commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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

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

@jklymak

jklymak commented Feb 11, 2020

Copy link
Copy Markdown
Member

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

pharshalp commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 reviewed Feb 11, 2020
View reviewed changes
Comment thread lib/matplotlib/tests/test_colorbar.py Outdated
jklymak
jklymak approved these changes Feb 11, 2020
View reviewed changes

@jklymak jklymak left a comment

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.

LGTM. Thanks a lot @pharshalp

anntzer
anntzer reviewed Feb 11, 2020
View reviewed changes
Comment thread lib/matplotlib/tests/test_colorbar.py Outdated
@anntzer anntzer added this to the v3.3.0 milestone Feb 11, 2020
anntzer
anntzer approved these changes Feb 11, 2020
View reviewed changes

@anntzer anntzer left a comment

Copy link
Copy Markdown
Contributor

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

pharshalp commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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