Opened 10 years ago
Closed 9 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: |
Description (last modified by )
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)
Change History (29)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Cc kcrisman added
comment:2 Changed 10 years ago by
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
- 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
- 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
- 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
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
Sure, this works.
comment:8 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:9 Changed 10 years ago by
- Cc ppurka added
comment:10 Changed 9 years ago by
Needs rebasing.
comment:11 Changed 9 years ago by
- 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 9 years ago by
- 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 9 years ago by
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 9 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:15 Changed 9 years ago by
- 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 9 years ago by
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 9 years ago by
- 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: ↓ 20 Changed 9 years ago by
Would you mind using the new ....:
doctest continuation instead of ...
?
comment:19 Changed 9 years ago by
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: ↓ 21 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
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 9 years ago by
comment:23 follow-up: ↓ 24 Changed 9 years ago by
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 9 years ago by
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: ↓ 26 Changed 9 years ago by
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 9 years ago by
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 9 years ago by
- Merged in set to sage-5.11.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
I'm not sure how to do a doctest for this one, since we can't check graphical output.