dashed arrows have arrowheads that are not solid

[jasongrout]

Right now, if you have a dashed arrow, the arrowhead is also drawn with a dashed linestyle, which really looks bad. Compare before and after for arrow((0,0), (1,1), linestyle='dashed').

This code works around a design issue in matplotlib. Currently, the specified linestyle is used to draw both the path and the arrowhead. If linestyle is 'dashed', this looks really odd. This code is from Jae-Joon Lee in response to a post to the matplotlib mailing list. See http://sourceforge.net/mailarchive/forum.php?thread_name=CAG%3DuJ%2Bnw2dE05P9TOXTz_zp-mGP3cY801vMH7yt6vgP9_WzU8w%40mail.gmail.com&forum_name=matplotlib-users

---

Apply attachment: trac_12852-dashedarrows.patch and attachment: trac_12852-review-rebase.patch.

CC: @kcrisman @orlitzky @ppurka

Component: graphics

Author: Jae-Joon Lee, Jason Grout, Michael Orlitzky

Reviewer: Michael Orlitzky, Karl-Dieter Crisman, Jason Grout

Merged: sage-5.11.beta2

Issue created by migration from https://trac.sagemath.org/ticket/12852

[jasongrout]

Attachment: trac_12852-dashedarrows.patch.gz

[jasongrout]

comment:1

I'm not sure how to do a doctest for this one, since we can't check graphical output.

[kcrisman]

comment:2

Will this cause our coverage to go down? Will it still accept all other optional arguments this way?

Doctest is not a problem; we just add a dashed arrow to the docs somewhere as a test. People should be checking the visual output in the "live" notebook in any case upon big changes in graphics handling.

I don't have enough familiarity with mpl to review this quickly, I'm sorry. I'm a little puzzled why it needs to be so big, but I guess it's hard to access just the arrowhead?

[jasongrout]

comment:3

I don't think it will cause doctest coverage to go down since it's a class inside of a function. It should accept all other optional arguments. Jae-Joon (one of the core matplotlib developers) is just using advanced functionality inside matplotlib that allows someone to modify how it draws things. The reason it needs to be so big is to work around a design issue in matplotlib. The cleanest fix is to fix matplotlib, but that isn't very easy because you have to change its design. I'm putting the patch here because I plan to apply it to my own server, and it might be good enough to go into Sage (as it does fix an ugly problem), and I don't have time to try to make the change to matplotlib and push that change there.

[jasongrout]

comment:4

(hence I'm setting it to needs work, as it probably at least needs a TESTS doctest explaining what is going on (that the arrowhead is solid even if the linestyle is dashed).

[orlitzky]

comment:5

Kind of lame, but hoping to make up for it with clever points:

sage: dashed = arrow((0,0), (1,1), linestyle='dashed')
sage: dashed.show(filename='dashed.eps')

The EPS format calls [6 6] 0 setdash to enable the dashes, and [] 0 setdash to disable them. Before the patch, we enable the dashes, stroke two objects, and then disable them:

gantu ~ $ cat dashed.eps | tr '\n' ' ' | grep -P -q 'setdash.*stroke.*stroke.*setdash'
gantu ~ $ echo $?
0

After the patch, we enable the dashes, stroke one object, and then disable the dashes:

gantu ~ $ cat dashed-patched.eps | tr '\n' ' ' | grep -P -q 'setdash.*stroke.*stroke.*setdash'
gantu ~ $ echo $?
1

[kcrisman]

comment:6

Interesting! Perhaps one could then call system things from Python to check it... but then one would have to use a temp file of the Sage type to make this legit.

[orlitzky]

comment:7

Sure, this works.

[kcrisman]

comment:10

Needs rebasing.

[kcrisman]

Reviewer: Michael Orlitzky

[kcrisman]

comment:11

I can confirm that Michael's patch documents this properly (we should totally use this trick in the future to document other plot fixes, if possible...), and that the arrowheads look nice now and that the eps files at any rate really do change properly (who knows about other formats). But I don't feel comfortable reviewing Jae-Joon's code. Jason, can you (or Michael) do that?

[kcrisman]

Changed author from Jae-Joon Lee, Jason Grout to Jae-Joon Lee, Jason Grout, Michael Orlitzky

[kcrisman]

comment:12

In fact, I assume Jason is okay with that other code...

[kcrisman]

Changed reviewer from Michael Orlitzky to Michael Orlitzky, Karl-Dieter Crisman, Jason Grout

[jasongrout]

comment:13

I think that's a safe assumption, since I put Jae-Joon's code up here and at least once applied it to my own server.

[jdemeyer]

Work Issues: rebase

[jdemeyer]

comment:15

The review patch doesn't apply:

applying /release/merger/patches/trac_12852-dashedarrows.patch
applying /release/merger/patches/sage-trac_12852-review.patch
patching file sage/plot/arrow.py
Hunk #2 FAILED at 300
1 out of 2 hunks FAILED -- saving rejects to file sage/plot/arrow.py.rej
abort: patch failed to apply

[kcrisman]

comment:16

Thanks for the reminder; I even said needs rebasing above. I guess I never actually rebased it? Oh, I did but didn't actually post it. Coming up.

[kcrisman]

Changed work issues from rebase to none

[kcrisman]

comment:17

Patchbot, apply trac_12852-dashedarrows.patch and trac_12852-review-rebase.patch

[jdemeyer]

comment:18

Would you mind using the new ....: doctest continuation instead of ...?

[jdemeyer]

comment:19

Also, you should use

tmp_filename(ext=".eps")

instead of

os.path.join(SAGE_TMP, 'arrow.eps')

[kcrisman]

comment:20

Would you mind using the new ....: doctest continuation instead of ...?

Well, it's not my review patch :-) but I can. I see that it's now in the developer guide, but it would have been helpful to send an email to sage-devel about this. Also, note that the example immediately above where this is encouraged still has the "old-style" continuation.

Also, you should use

Same comment, though I don't think we deprecated SAGE_TMP yet, did we?

Anyway, coming right up.

[jdemeyer]

comment:21

Replying to @kcrisman:

it would have been helpful to send an email to sage-devel about this.

I'm pretty sure I did that...

Same comment, though I don't think we deprecated SAGE_TMP yet, did we?

No, and it's not clear that we should do this. In some (rare) cases, it might be needed to access SAGE_TMP directly. Also it would imply changing all places where SAGE_TMP is still used.

[jdemeyer]

comment:22

SAGE_TMP is nowhere mentioned in the Develop's Guide, while tmp_filename() is mentioned in http://sagemath.org/doc/developer/conventions.html#further-conventions-for-automated-testing-of-examples

[kcrisman]

Attachment: trac_12852-review-rebase.patch.gz

[kcrisman]

comment:23

Should be all set.

Where did the original review patch go? Are we in the habit of deleting patches now? I've never seen this before... sometimes I wish I could delete my own patches, but I can't even do that :-)

[jdemeyer]

comment:24

Replying to @kcrisman:

Where did the original review patch go?

I removed it in order to avoid possible confusion, but perhaps I should not have done that.

[orlitzky]

comment:25

After a year of needs_review you guys decide to work on this while I'm out of the office huh? Anything left you need me to do?

[kcrisman]

comment:26

After a year of needs_review you guys decide to work on this while I'm out of the office huh? Anything left you need me to do?

Well, it just depends on when one has time :-) and I just tried it out, it needed rebasing, and I had some time...

[jdemeyer]

Merged: sage-5.11.beta2