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

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

Merged
merged 1 commit into from
Jun 8, 2019

Conversation

bsipocz
Copy link
Contributor

@bsipocz bsipocz commented Jun 4, 2019

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.

@tacaswell tacaswell added this to the v2.2.5 milestone Jun 4, 2019
@jklymak
Copy link
Member

jklymak commented Jun 5, 2019

Can you reproduce the error in matplotlib somehow? I can’t really tell what the issue is from the astropy example.

@bsipocz
Copy link
Contributor Author

bsipocz commented Jun 5, 2019

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

jklymak commented Jun 5, 2019

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
Copy link
Contributor

@anntzer anntzer left a comment

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

dstansby commented Jun 5, 2019

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
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
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
Copy link
Contributor Author

bsipocz commented Jun 8, 2019

@tacaswell - I'm happy about the widening communication channels, too. I suppose it's a side effect of being upstream that we mostly report about breakage, but at least we start to run into them while testing against the dev version, even though this instance it wasn't the case.

Also, I was looking into to add a test here, too, just haven't yet got to a point to identify what I could use in Line2D that is different. If I don't open a PR by the end of the weekend about it, then I will have given up on it.

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.

None yet

5 participants