FIX: return points rather than path to fix regression by bsipocz · Pull Request #14451 · matplotlib/matplotlib · GitHub
Skip to content

FIX: return points rather than path to fix regression - #14451

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
bsipocz:keep_only_points
Jun 8, 2019
Merged

tacaswell merged 1 commit into
matplotlib:masterfrom
bsipocz:keep_only_points

Conversation

@bsipocz

@bsipocz bsipocz commented Jun 4, 2019

Copy link
Copy Markdown
Contributor

Regression in an astropy plotting example (astropy/astropy#8792) is due to #11407

Locally this PR fixes the issue and the test seem to be still working, but I didn't run the whole suite locally.

Please advise what kind of test is preferred to be added for this.

@bsipocz
bsipocz force-pushed the keep_only_points branch from dd65cfe to cd28dd6 Compare June 4, 2019 23:55
@tacaswell tacaswell added this to the v2.2.5 milestone Jun 4, 2019
@jklymak

jklymak commented Jun 5, 2019

Copy link
Copy Markdown
Member

@bsipocz

bsipocz commented Jun 5, 2019

Copy link
Copy Markdown
Contributor Author

The problem is that we wanted to plot points rather than paths. This example from the issue in fact doesn't use astropy:

import numpy as np
import matplotlib.pyplot as plt

plt.figure(figsize=(8, 4))
plt.subplot(111, projection="aitoff")

x = [0, np.pi/4, np.pi/2]
y = [0, np.pi/4, 3*np.pi/8]

plt.scatter(x, y, color='tab:orange')

plt.plot(x, y,
         marker='o', linestyle='none', markersize=2, alpha=0.3)

@jklymak

jklymak commented Jun 5, 2019

Copy link
Copy Markdown
Member

Great - I guess this is fine - if @anntzer had a reason for changing paths to points, I'm sure he'd have written a test to make sure we didn't re-break it on him 😛 But I actually suspect this was just a typo.

I'm not 100% clear why this requires a projection to show up. A test would be very useful - if there was a way to make it not be an image test, even better, but sometimes those are unavoidable.

@jklymak
jklymak requested a review from anntzer June 5, 2019 01:07

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

I clearly typoed in #11407, sorry about that.

@dstansby

dstansby commented Jun 5, 2019

Copy link
Copy Markdown
Member

Is it possible to get the Line2D objects returned by plot() and do some checks there? Otherwise I think this might need an image test (it might be possible to modify an existing custom projection image test to exercise this though).

@tacaswell

Copy link
Copy Markdown
Member

Merged to get the back ports going for 3.1.1, will open an issue to add tests.

@tacaswell tacaswell mentioned this pull request Jun 8, 2019
@tacaswell

Copy link
Copy Markdown
Member

Thank you @bsipocz !

I am very happy that there is cross-project communication / collaboration going on (which to be fair has mostly been astropy telling Matplotlib we broke you ....).

@bsipocz

bsipocz commented Jun 8, 2019

Copy link
Copy Markdown
Contributor Author

timhoffm added a commit that referenced this pull request Jun 9, 2019
…451-on-v3.1.x

Backport PR #14451 on branch v3.1.x (FIX: return points rather than path to fix regression)
tacaswell added a commit that referenced this pull request Oct 22, 2019
…451-on-v2.2.x

Backport PR #14451 on branch v2.2.x (FIX: return points rather than path to fix regression)
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.

5 participants