Skip to content
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.

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

Fixes matplotlib/matplotlib#1235 #6110

Merged
merged 1 commit into from Mar 7, 2016

Conversation

nansonzheng
Copy link

Credit to @dashed, fixes #1235, supercedes #2857

The problem is that _find_best_position does not receive positions of (transformed) points from _auto_legend_data. We attempted to fix it by adding the offsets of points, after preparing the points and transforming them, and calling legendBox.count_contains on offsets.

@jenshnielsen jenshnielsen added this to the 2.0 (style change major release) milestone Mar 5, 2016
@dashed
Copy link
Contributor

dashed commented Mar 6, 2016

AppVeyor builds are failing; but I cannot make sense of the logs. Can someone enlighten me?

@tacaswell
Copy link
Member

See #6115 That is probably unrelated to your work

@@ -737,6 +737,8 @@ def _auto_legend_data(self):
ax = self.parent
bboxes = []
lines = []
vertices = []
Copy link
Member

Choose a reason for hiding this comment

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

Do not need this, it looks like vertices just gets re-assigned below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

@tacaswell
Copy link
Member

Other that 1 very minor style point 👍

plt.gca().set_ylim(-0.5, 2.2)


@image_comparison(baseline_images=['not_covering_scatter_transform'], extensions=['png'])
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is over 80 characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

I noticed the rest of the test file violates this 😉 Can be amended in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine putting off fixing pep8 in the tests.

Copy link
Member

Choose a reason for hiding this comment

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

It's actually fixed in another PR that I might just give a little nudge to some time...

@dashed
Copy link
Contributor

dashed commented Mar 7, 2016

@tacaswell I rebased against master that has #6116, and seems to work. Unsure what happened here: https://ci.appveyor.com/project/mdboom/matplotlib/build/1.0.985/job/4u3na3cignvxhg2n

@tacaswell
Copy link
Member

That is a known transient failure.

tacaswell added a commit that referenced this pull request Mar 7, 2016
@tacaswell tacaswell merged commit 01c73b4 into matplotlib:master Mar 7, 2016
tacaswell added a commit that referenced this pull request Mar 7, 2016
@tacaswell
Copy link
Member

backported to v2.x as 869cc9d

@dashed
Copy link
Contributor

dashed commented Mar 7, 2016

Thanks @tacaswell 👍

@QuLogic QuLogic mentioned this pull request Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Legend placement bug
6 participants