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

Bugfix hyperbolic_arc and hyperbolic_polygon #19217

Closed
sagetrac-skraemer mannequin opened this issue Sep 15, 2015 · 16 comments
Closed

Bugfix hyperbolic_arc and hyperbolic_polygon #19217

sagetrac-skraemer mannequin opened this issue Sep 15, 2015 · 16 comments

Comments

@sagetrac-skraemer
Copy link
Mannequin

sagetrac-skraemer mannequin commented Sep 15, 2015

If you draw an hyperbolic arc between two points with almost the same real part, it may result in a wrong arc.

You can see it as follows:

g = hyperbolic_triangle(-1+I, 1.0000000000001+I, 1000*I+1, fill = true);
g.set_axes_range(-1.5,1.5,-.5,2.5)
g.show()

CC: @videlec

Component: graphics

Keywords: hyperbolic_arc, hyperbolic_polygon

Author: Stefan Kraemer

Branch/Commit: adec9d2

Reviewer: Vincent Delecroix

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

@sagetrac-skraemer sagetrac-skraemer mannequin added this to the sage-6.9 milestone Sep 15, 2015
@sagetrac-skraemer

This comment has been minimized.

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 15, 2015

Changed keywords from none to hyperbolic_arc, hyperbolic_polygon

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 15, 2015

Author: skraemer

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 15, 2015

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 15, 2015

Commit: 855f507

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 15, 2015

New commits:

855f507Bugfix for hyperbolic_arc and hyperbolic_polygon

@videlec
Copy link
Contributor

videlec commented Sep 15, 2015

comment:5

Hello,

Why 10-3? Moreover it can be very wrong if the imaginary part is very small. Just try

sage: hyperbolic_triangle(0, 0.0001, 0.0001*I)

The above example just works fine without your patch.

Vincent

@videlec
Copy link
Contributor

videlec commented Sep 15, 2015

Reviewer: Vincent Delecroix

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 25, 2015

comment:6

Hello Vincent,

thanks for reviewing my first patch!

Would a smaller boundary for the comparison be more eligible? Let's say 10-12? Of course, the analogue example of ours will still produce a wrong result:

g = hyperbolic_triangle(0, 10**(-12),10**(-12)*I);g

But scaled to a larger region of hyperbolic geometry, it is quite hard to see:

g.set_axes_range(0,10**(-10),0,10**(-10));g

Or do you know a better way to compare a value to zero in sage?

best regards,
Stefan

@videlec
Copy link
Contributor

videlec commented Sep 25, 2015

comment:7

Hello,

Putting a smaller value would not solve anything. The main problem is that this subtelty should depend on the window parameters (i.e. xmin, xmax, ymin, ymax). Hence decided at the time you generate the image.

I have no miracle to propose.

Vincent

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2015

Changed commit from 855f507 to adec9d2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

b3b9821Bugfix hyperbolic_arc and hyperbolic_polygon
adec9d2Bugfix for hyperbolic_arc and hyperbolic_polygon

@sagetrac-skraemer
Copy link
Mannequin Author

sagetrac-skraemer mannequin commented Sep 28, 2015

comment:9

Hello,

I guess, something went wrong with git. I am sorry for this. Please let me know, if it does not work.

I had a new idea, how to handle this issue: Instead of checking, whether the points are above each other, one could check, how much the line connecting the points differ by the circle through the points. A simple test for this is to look at the quotient of the length of the connecting line and the radius of the circle. If this quotient is smaller than 0.1, we choose a line. The parameter .1 was chosen by experiment.

In this version the reported bug does not appear and your examples work also.

best regards,
Stefan

@videlec
Copy link
Contributor

videlec commented Oct 5, 2015

Changed author from skraemer to Stefan Kraemer

@videlec
Copy link
Contributor

videlec commented Oct 5, 2015

comment:10

Hello,

Some trac administrative things:

  • Once you are done with some modification, you should switch the status to needs review (it was in needs work).
  • the Authors field should be with plain names (not the login)
    This is fine for now.

Your solution is great! I set to positive review and it will be soonly merge in the development release.

Vincent

@vbraun
Copy link
Member

vbraun commented Oct 14, 2015

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

2 participants