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

arrows are shortened too much #12836

Closed
jasongrout opened this issue Apr 12, 2012 · 8 comments
Closed

arrows are shortened too much #12836

jasongrout opened this issue Apr 12, 2012 · 8 comments

Comments

@jasongrout
Copy link
Member

Apparently 3 years ago, when I made an update patch for matplotlib, I put in some default shortening of arrows. Matplotlib already shortens its arrows to account for line width, so with my shortening and matplotlib's shortening, arrows were always way shorter than they were intended. This patch removes Sage's default shortening of arrows.

CC: @kcrisman

Component: graphics

Author: Jason Grout

Reviewer: Michael Orlitzky

Merged: sage-5.1.beta1

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

@jasongrout
Copy link
Member Author

comment:1

Attachment: trac-12836-arrow-shorten.patch.gz

Needless to say, since it's a graphics change, people better look at it so people don't get terribly offended that there arrows look a little better :).

@orlitzky
Copy link
Contributor

Attachment: before.png

@orlitzky
Copy link
Contributor

Attachment: after.png

@orlitzky
Copy link
Contributor

Add a doctest.

@orlitzky
Copy link
Contributor

comment:2

Attachment: sage-trac_12836-review.patch.gz

This change looks fine. The before/after.png show an arrow from the origin to (1,1) before and after the patch. I added a doctest that creates two arrows with different line widths, and checks that their shrinkA and shrinkB values are the same.

There's a chance I'm doing something dumb in the doctest, but positive review otherwise.

@orlitzky
Copy link
Contributor

Reviewer: Michael Orlitzky

@jasongrout
Copy link
Member Author

comment:3

Your doctest patch looks good.

@jdemeyer
Copy link

Merged: sage-5.1.beta1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants