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

FIX label vertical alignment can now be specified by NelleV · Pull Request #7970 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

FIX label vertical alignment can now be specified - #7970

Closed
NelleV wants to merge 2 commits into
matplotlib:masterfrom
NelleV:fix_7905
ConversationCommits (2)ChecksFiles changed
Closed

FIX label vertical alignment can now be specified#7970
NelleV wants to merge 2 commits into
matplotlib:masterfrom
NelleV:fix_7905

Conversation

NelleV commented Jan 28, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

This patch adds a new rcParams allowing to set label vertical alignment. The
sole reason for the existance of this new parameter is to allow user to reset
the style to before 2.0 for testing purposes.

This patch provides a fix to a very pick backward compatibility break. Unfortunately, the
patch that introduces this break also regenerated the test images. This leads to the introduction of a private _test_class stylesheet, corresponding to our current test suite, and a classic stylesheet which is backward compatible.

refs #7905

TODO

  • Document this new parameter

NelleV added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 28, 2017
NelleV added this to the 2.0.1 (next bug fix release) milestone Jan 28, 2017
tacaswell reviewed Jan 28, 2017
View reviewed changes
Comment thread doc/users/dflt_style_changes.rst Outdated
See :ref:`customizing-with-matplotlibrc-files` for details about how to
persistently and selectively revert many of these changes.

.. note::

tacaswell Jan 28, 2017

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

I would give this a 'YTick alignment' section in the 'Tick Label Formatting' section. We don't have completely throw our selves under the bus in the first section. I don't think this needs to be called out in a note, merging it into the paragraph above would be enough.

NelleV Jan 28, 2017

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

👍 on the name and section. Finding a better name was on my todo list.

NelleV added 2 commits January 28, 2017 11:27
FIX ytick vertical alignment can now be specified
e5c2dda 
This patch adds a new rcParams allowing to set label vertical alignment. The
sole reason for the existance of this new parameter is to allow user to reset
the style to before 2.0 for testing purposes.

closes #7905
MAINT created a new private _classic_test stylesheet
4842eaa 
This allows the public classic stylesheet to be 'more' backward compatible
NelleV changed the title [WIP] FIX label vertical alignment can now be specified [MRG] FIX label vertical alignment can now be specified Jan 28, 2017

NelleV commented Jan 28, 2017

Copy link
Copy Markdown
Member Author

attn @matplotlib/developers
This patch introduces a new "private" stylesheet called _classic_test. The "classic" stylesheet can now be 'more' backward compatible without breaking our test suite.

anntzer commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

What is the argument for adding yet another private rcparam (which the codebase will have to keepa round for a while) rather than regenerating the test suite images (which is a one shot thing)?

NelleV commented Jan 28, 2017

Copy link
Copy Markdown
Member Author

@anntzer It is not private. It is actually used in the classic stylesheet. It is for backward compatibility.
See #refs #7905

anntzer commented Jan 28, 2017

Copy link
Copy Markdown
Contributor

Ah, I see, sorry for the noise.
Still, could we just not add the "_classic_test" style and regenerate the test images with the new "ytick.alignment: center" rcparam?

NelleV commented Jan 28, 2017

Copy link
Copy Markdown
Member Author

It's around 350 images to regenerate. It's makes the git repository grow massively :(

I'll let @tacaswell give the final word on this.

NelleV mentioned this pull request Jan 29, 2017
Merged

WeatherGod commented Jan 30, 2017

Copy link
Copy Markdown
Member

For consistency's sake, we should also have an xtick version of this parameter.

I am disappointed that we seemed to have updated our baseline images, thereby causing this discrepancy between our classic style used by our tests and the classic style used by our users, but I agree that it would be much more harmful to regenerate these images for such a tiny change. We run the risk of a more serious error during the regeneration process.

tacaswell commented Jan 30, 2017

Copy link
Copy Markdown
Member

I am still in favor of the extra style sheet and 👍 on doing x as well.

QuLogic commented Feb 6, 2017

Copy link
Copy Markdown
Member

With the pytest migration, it would be simpler if you rebased on and targetted v2.0.x directly, then resolving the conflict upwards would be less work. Could you do so and add the 'x' version that @tacaswell requested?

This was referenced Feb 9, 2017
Merged
Merged
phobson mentioned this pull request Feb 13, 2017
Merged

NelleV commented Feb 15, 2017

Copy link
Copy Markdown
Member Author

This patch is going to be a mess to apply both on master and v2.0.x
I suspect we may have to create two pull requests.

NelleV mentioned this pull request Feb 15, 2017
Merged
2 tasks done

NelleV commented Feb 15, 2017

Copy link
Copy Markdown
Member Author

I am closing this in favor of #8081 : same patch, targetting v2.0.x.

NelleV closed this Feb 15, 2017
QuLogic changed the title [MRG] FIX label vertical alignment can now be specified FIX label vertical alignment can now be specified Apr 30, 2017
ImportanceOfBeingErnest mentioned this pull request Feb 27, 2020
Closed
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

tacaswell tacaswell left review comments

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: duplicate

Projects

None yet

Milestone

v2.0.1

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL