Opened 11 years ago
Closed 3 years ago
#3859 closed defect (fixed)
Line's corner_cutoff is poorly documented, and buggy
Reported by:  mclean  Owned by:  was 

Priority:  major  Milestone:  sage7.1 
Component:  graphics  Keywords:  plot3d, Line, corner_cutoff, smoothing 
Cc:  jason, kcrisman  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  KarlDieter Crisman, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  bc5b08a (Commits)  Commit:  bc5b08a9516f877b5a82f286a39f2779b8518e2d 
Dependencies:  Stopgaps: 
Description
sage.plot.plot3d.shapes2.Line defines a corner_cutoff parameter.
It functions to smooth lines passed to it if the cosine of the angle is greater than corner_cutoff (why???  I'm filing another ticket about this).
We want to be able to turn the smoothing of lines off, which would happen if corner_cutoff = 1 worked.
Example of breakage: (line3d calles sage.plot.plot3d.shapes2.Line)
line3d([[1, 3, 369.26], [1, 10.11, 125.33], [0.21, 10.13, 99.42]], corner_cutoff = 1)
A doctest sage: from sage.plot.plot3d.shapes2 import Line
sage: Line([(0,0,0),(1,0,0),(2,1,0),(0,1,0)], corner_cutoff=1).corners() [(0, 0, 0), (1, 0, 0), (2, 1, 0)]
Works, but calling line3d or Line with these parameters fails with NoneType? object unsubscriptable.
Change History (29)
comment:1 Changed 11 years ago by
comment:2 Changed 9 years ago by
 Cc jason added
 Report Upstream set to N/A
comment:3 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:4 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:5 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:6 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:7 Changed 4 years ago by
 Branch set to u/chapoton/3859
 Commit set to 6ff1e76a73b72c651d3a79b7c7c3f313b606372c
 Status changed from new to needs_review
Here is a proposal, that corrects several minor wrong points in the code of Line and add more doc.
New commits:
6ff1e76  trac #3859 solving issues with Line in 3d

comment:8 Changed 4 years ago by
 Cc kcrisman added
See also http://ask.sagemath.org/question/25609/howtofindthefullargumentlistofabuiltinfunction/  amazingly, I have never heard of this!
comment:9 Changed 4 years ago by
 Milestone changed from sage6.4 to sage6.5
comment:10 Changed 4 years ago by
 Milestone changed from sage6.5 to sage6.6
comment:11 Changed 4 years ago by
Hello, review, anybody ?
comment:12 Changed 4 years ago by
Either I don't understand the description of what corner_cutoff
is supposed to do, or the first example from the ticket's description still doesn't work for me:
sage: line3d([[1, 3, 369.26], [1, 10.11, 125.33], [0.21, 10.13, 99.42]], corner_cutoff = 1)
produces a smooth line in jmol.
comment:13 Changed 4 years ago by
 Milestone changed from sage6.6 to sage6.7
 Status changed from needs_review to needs_work
I guess there is still a problem.
comment:14 Changed 4 years ago by
 Milestone changed from sage6.7 to sage6.8
comment:15 Changed 3 years ago by
 Commit changed from 6ff1e76a73b72c651d3a79b7c7c3f313b606372c to 10c880c4db6e1c621c96419dbcd33e469033d2c6
comment:16 Changed 3 years ago by
 Status changed from needs_work to needs_review
This should be better now:
corner_cutoff = 1
: no smoothing
corner_cutoff = 1
: all points smoothed
and smoothing is performed in general if the angle is close enough to pi, meaning that successive segments are close to being aligned.
comment:17 Changed 3 years ago by
 Milestone changed from sage6.8 to sage6.10
comment:18 Changed 3 years ago by
ping ? should be an easy review !
comment:19 Changed 3 years ago by
PING ? nobody out there ?
comment:20 Changed 3 years ago by
Well, someone is out there but they don't always have time... My only Sage time has been with sagenb lately. But I felt sorry for your ping so I try to make time tonight.
The parameter ``corner_cutoff`` is a bound for the cosine of the angle made by two successive segments. This angle is close to `\pi` if the two successive segments are almost aligned and close to `0` if the path has a strong peak.
Maybe one could add parenthetically "(and hence the cosine is close to 1)" or the like as appropriate in the two places this occurs?
+ sage: N = 11 + sage: c = 0.4 + sage: sum([Line([(i,1,0), (i,0,0), (i,cos(2*pi*i/N), sin(2*pi*i/N))], + ....: corner_cutoff=c, + ....: color='red' if cos(2*pi*i/N)>=c else 'blue') + ....: for i in range(N+1)])
Superuseful! I made it into an interact to test things, very nice.
I do have a question about this. Why do you have to change the way this works? Why not leave the bounds at 1 and 1 and not switch them? I hate to say the magic word deprecation, and in any case this is just a flatout change.
For instance, I would say that the angle is nearly zero if the segments "keep on going" and is close to 180 degrees if the segments change direction (peak). So the original seems closer to my thinking  it's not the angle at the actual point of contact of the two segments, but rather the angle between the vectors formed by the two (directed) line segments that is in question.
Does that make sense?
I also have to admit that with #3861 that the output of
Line([(0,0,0),(1,0,0),(2,1,0),(0,1,0)],corner_cutoff=.5) # current ticket
is bizarre and does not look like the 2d version style in any case.
Anyway, otherwise this seems to work as advertised. Should end users have access to max_len
in Line
? Currently I don't think that's really possible. But maybe that's okay.
comment:21 Changed 3 years ago by
 Commit changed from 10c880c4db6e1c621c96419dbcd33e469033d2c6 to bc5b08a9516f877b5a82f286a39f2779b8518e2d
comment:22 Changed 3 years ago by
Thanks for having a look. I know well that time is precious.
I have now, as required, changed back to the original convention that angle 0 means that the path goes straight. I hope I have made no error in doing that.
Maybe this is good enough, for a 7 years old ticket ?
comment:23 Changed 3 years ago by
 Milestone changed from sage6.10 to sage7.0
comment:24 Changed 3 years ago by
*ping* ?
comment:25 Changed 3 years ago by
 Milestone changed from sage7.0 to sage7.1
comment:26 Changed 3 years ago by
 Reviewers set to KarlDieter Crisman, Travis Scrimshaw
This LGTM, but I'd like to see if KarlDieter has any more comments before we set this to a positive review.
comment:27 Changed 3 years ago by
If Travis can confirm that the original convention is now followed and that the awesome examples still look as they should, please do put it to positive! I just continue to have no time for anything involving branches :( somehow that takes me much more time than e.g. reporting tickets.
comment:28 Changed 3 years ago by
 Status changed from needs_review to positive_review
AFAIK it is correct. At the very least, it is better than the current behavior of erroring out.
comment:29 Changed 3 years ago by
 Branch changed from u/chapoton/3859 to bc5b08a9516f877b5a82f286a39f2779b8518e2d
 Resolution set to fixed
 Status changed from positive_review to closed
See also the related: http://trac.sagemath.org/sage_trac/ticket/3861