Opened 5 years ago

Closed 5 years ago

#19217 closed defect (fixed)

Bugfix hyperbolic_arc and hyperbolic_polygon

Reported by: skraemer Owned by:
Priority: trivial Milestone: sage-6.9
Component: graphics Keywords: hyperbolic_arc, hyperbolic_polygon
Cc: vdelecroix Merged in:
Authors: Stefan Kraemer Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: adec9d2 (Commits) Commit: adec9d2014a81c41e98e49b2f172e1f343739196
Dependencies: Stopgaps:

Description (last modified by skraemer)

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

Change History (11)

comment:1 Changed 5 years ago by skraemer

  • Authors set to skraemer
  • Component changed from PLEASE CHANGE to graphics
  • Description modified (diff)
  • Keywords hyperbolic_arc hyperbolic_polygon added
  • Priority changed from major to trivial
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 5 years ago by skraemer

  • Branch set to u/skraemer/bugfix_hyperbolic_arc_and_hyperbolic_polygon

comment:3 Changed 5 years ago by skraemer

  • Commit set to 855f5078cd89e78bedc1edd18bc700f1c837ce07
  • Status changed from new to needs_review

New commits:

855f507Bugfix for hyperbolic_arc and hyperbolic_polygon

comment:4 Changed 5 years ago by skraemer

  • Cc vdelecroix added

comment:5 Changed 5 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

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

comment:6 Changed 5 years ago by skraemer

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

comment:7 Changed 5 years ago by vdelecroix

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

comment:8 Changed 5 years ago by git

  • Commit changed from 855f5078cd89e78bedc1edd18bc700f1c837ce07 to adec9d2014a81c41e98e49b2f172e1f343739196

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

b3b9821Bugfix hyperbolic_arc and hyperbolic_polygon
adec9d2Bugfix for hyperbolic_arc and hyperbolic_polygon

comment:9 Changed 5 years ago by skraemer

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

comment:10 Changed 5 years ago by vdelecroix

  • Authors changed from skraemer to Stefan Kraemer
  • Status changed from needs_work to positive_review

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

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/skraemer/bugfix_hyperbolic_arc_and_hyperbolic_polygon to adec9d2014a81c41e98e49b2f172e1f343739196
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.