X Tutup
Skip to content

Fixes matplotlib/matplotlib#1235#6110

Merged
tacaswell merged 1 commit intomatplotlib:masterfrom
nansonzheng:1235-fix-proposal
Mar 7, 2016
Merged

Fixes matplotlib/matplotlib#1235#6110
tacaswell merged 1 commit intomatplotlib:masterfrom
nansonzheng:1235-fix-proposal

Conversation

@nansonzheng
Copy link
Copy Markdown

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

See #6115 That is probably unrelated to your work

ax = self.parent
bboxes = []
lines = []
vertices = []
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.

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

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.

Fixed.

@tacaswell
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 dashed force-pushed the 1235-fix-proposal branch from 035b33b to da49463 Compare March 7, 2016 02:55
@dashed
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

backported to v2.x as 869cc9d

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

X Tutup