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

Fixed not being able to set vertical/horizontal alignments in polar graphs · Pull Request #10792 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Fixed not being able to set vertical/horizontal alignments in polar graphs - #10792

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
jinshifen33:bugfix-for-issue-10105
Apr 23, 2018
ConversationCommits (1)ChecksFiles changed
Merged

Fixed not being able to set vertical/horizontal alignments in polar graphs#10792
jklymak merged 1 commit into
matplotlib:masterfrom
jinshifen33:bugfix-for-issue-10105

Conversation

ghost commented Mar 15, 2018 •
edited by ghost
Loading

Copy link
Copy Markdown

PR Summary

Previously polar graphs could not have their vertical/horizontal alignments set by set_rgrids(), this fix should re-enable this functionality.

Issue #10105

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant

ghost commented Mar 16, 2018

Copy link
Copy Markdown
Author

I believe this pull request is what caused this regression.

pbregener commented Mar 23, 2018

Copy link
Copy Markdown

Would love to see this simple patch backported to v2.2.x as well 👍

OD1995 commented Mar 27, 2018

Copy link
Copy Markdown

@charlesruan @pbregener is this fix live? If so, how can I take advantage of the changes?

pbregener commented Mar 27, 2018

Copy link
Copy Markdown

This MR is not merged, yet, so it's not "live" in the sense that it is part of any release or even current master. I guess the 5 commits should be squashed into one with a sensible commit message and then some maintainer should review and eventually merge it.

ImportanceOfBeingErnest commented Mar 27, 2018

Copy link
Copy Markdown
Member

There is also this regression on polar plots: #10882
Are they related?

jklymak added this to the v3.0 milestone Mar 27, 2018
phobson approved these changes Mar 27, 2018
View reviewed changes

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

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

Thanks for the PR. It looks like this works as expected.

jklymak commented Mar 27, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

PRs need two reviews. This looks reasonable, but it would be nice to know what PR the regression came in from and whether this was just an oversight or a design decision.

pbregener commented Mar 27, 2018

Copy link
Copy Markdown

@jklymak This goes back to #9068 and therefore v2.1.0. Also see the original issue for this: #10105

jklymak commented Mar 27, 2018

Copy link
Copy Markdown
Member

Thanks. ping @QuLogic who is the local polar expert.

jklymak approved these changes Mar 27, 2018
View reviewed changes

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.

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

This looks fine to me, but reluctant to merge w/o @QuLogic having a look..

dopplershift commented Mar 27, 2018

Copy link
Copy Markdown
Contributor

If this really is a regression, it should probably be milestoned for v2.2.3

jklymak commented Mar 27, 2018

Copy link
Copy Markdown
Member

Its a 2.1 regression. Not sure on the policy there.

pbregener commented Mar 27, 2018

Copy link
Copy Markdown

It is a regression and as mentioned above, I would love to see this in v2.2.x. Should be really easy to backport, too.

Not sure about how matplotlib usually handles this, but I guess it would be nice to squash the five commits for this fairly trivial change into one @charlesruan

dopplershift commented Mar 27, 2018

Copy link
Copy Markdown
Contributor

@tacaswell said:

I propose the following criteria for backporting PRs to the 2.2.x branch. We always will backport

  • critical bug fixes (segfault, failure to import, things that the user can not work around)
  • fixes for regressions against 2.0 or 2.1

This would seem to fit.

dopplershift modified the milestones: v3.0, v2.2.3 Mar 27, 2018
br-Zhang force-pushed the bugfix-for-issue-10105 branch from 965e5d9 to ad70feb Compare March 27, 2018 21:09

ghost commented Mar 27, 2018

Copy link
Copy Markdown
Author

Commits have been squashed

pbregener commented Apr 16, 2018

Copy link
Copy Markdown

ping @jklymak and especially @QuLogic for an update on this PR?

jklymak merged commit b33aabc into matplotlib:master Apr 23, 2018
lumberbot-app Bot pushed a commit that referenced this pull request Apr 23, 2018
Backport PR #10792: Fixed not being able to set vertical/horizontal a…
4034b85 
…lignments in polar graphs
lumberbot-app Bot mentioned this pull request Apr 23, 2018
Merged

phobson commented Apr 24, 2018

Copy link
Copy Markdown
Member

@charlesruan thanks so much for this PR and your patience with the process!

jklymak added a commit that referenced this pull request Apr 24, 2018
Merge pull request #11114 from matplotlib/auto-backport-of-pr-10792
d1214fa 
Backport PR #10792 on branch v2.2.x
pbregener mentioned this pull request May 22, 2018
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

No reviews

Assignees

No one assigned

Labels

topic: polar

Projects

None yet

Milestone

v2.2.3

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL