Opened 10 years ago

Closed 8 years ago

#12852 closed enhancement (fixed)

dashed arrows have arrowheads that are not solid

Reported by: jason Owned by: jason, was
Priority: major Milestone: sage-5.11
Component: graphics Keywords:
Cc: kcrisman, mjo, ppurka Merged in: sage-5.11.beta2
Authors: Jae-Joon Lee, Jason Grout, Michael Orlitzky Reviewers: Michael Orlitzky, Karl-Dieter Crisman, Jason Grout
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by kcrisman)

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 trac_12852-dashedarrows.patch and trac_12852-review-rebase.patch.

Attachments (2)

trac_12852-dashedarrows.patch (3.1 KB) - added by jason 10 years ago.
trac_12852-review-rebase.patch (1.7 KB) - added by kcrisman 8 years ago.

Download all attachments as: .zip

Change History (29)

Changed 10 years ago by jason

comment:1 Changed 10 years ago by jason

  • Cc kcrisman added

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

comment:2 Changed 10 years ago by kcrisman

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?

comment:3 Changed 10 years ago by jason

  • Status changed from new to needs_review

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.

comment:4 Changed 10 years ago by jason

  • Status changed from needs_review to needs_work

(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).

comment:5 Changed 10 years ago by mjo

  • Cc mjo added

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

comment:6 Changed 10 years ago by kcrisman

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.

comment:7 Changed 10 years ago by mjo

Sure, this works.

comment:8 Changed 9 years ago by mjo

  • Status changed from needs_work to needs_review

comment:9 Changed 9 years ago by ppurka

  • Cc ppurka added

comment:10 Changed 8 years ago by kcrisman

Needs rebasing.

comment:11 Changed 8 years ago by kcrisman

  • Authors changed from Jae-Joon Lee, Jason Grout to Jae-Joon Lee, Jason Grout, Michael Orlitzky
  • Reviewers set to Michael Orlitzky

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?

comment:12 Changed 8 years ago by kcrisman

  • Reviewers changed from Michael Orlitzky to Michael Orlitzky, Karl-Dieter Crisman, Jason Grout
  • Status changed from needs_review to positive_review

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

comment:13 Changed 8 years ago by jason

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.

comment:14 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:15 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase

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

comment:16 Changed 8 years ago by kcrisman

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.

comment:17 Changed 8 years ago by kcrisman

  • Description modified (diff)
  • Status changed from needs_work to positive_review
  • Work issues rebase deleted

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

comment:18 follow-up: Changed 8 years ago by jdemeyer

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

comment:19 Changed 8 years ago by jdemeyer

Also, you should use

tmp_filename(ext=".eps")

instead of

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

comment:20 in reply to: ↑ 18 ; follow-up: Changed 8 years ago by kcrisman

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.

comment:21 in reply to: ↑ 20 Changed 8 years ago by jdemeyer

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.

comment:22 Changed 8 years ago by jdemeyer

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

Changed 8 years ago by kcrisman

comment:23 follow-up: Changed 8 years ago by kcrisman

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 :-)

comment:24 in reply to: ↑ 23 Changed 8 years ago by jdemeyer

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.

comment:25 follow-up: Changed 8 years ago by mjo

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?

comment:26 in reply to: ↑ 25 Changed 8 years ago by kcrisman

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

comment:27 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.