Sage: Ticket #3859: Line's corner_cutoff is poorly documented, and buggy
https://trac.sagemath.org/ticket/3859
<p>
sage.plot.plot3d.shapes2.Line defines a corner_cutoff parameter.
</p>
<p>
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).
</p>
<p>
We want to be able to turn the smoothing of lines off, which would happen if corner_cutoff = 1 worked.
</p>
<p>
Example of breakage: (line3d calles sage.plot.plot3d.shapes2.Line)
</p>
<pre class="wiki">line3d([[-1, 3, 369.26], [-1, -10.11, 125.33], [0.21, -10.13, 99.42]], corner_cutoff = 1)
</pre><p>
A doctest sage: from sage.plot.plot3d.shapes2 import Line
</p>
<pre class="wiki"> 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)]
</pre><p>
Works, but calling line3d or Line with these parameters fails with <a class="missing wiki">NoneType?</a> object unsubscriptable.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/3859
Trac 1.1.6mcleanThu, 14 Aug 2008 22:28:11 GMT
https://trac.sagemath.org/ticket/3859#comment:1
https://trac.sagemath.org/ticket/3859#comment:1
<p>
See also the related:
<a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/3861"><span class="icon"></span>http://trac.sagemath.org/sage_trac/ticket/3861</a>
</p>
TicketjasonTue, 27 Apr 2010 15:26:25 GMTcc, upstream set
https://trac.sagemath.org/ticket/3859#comment:2
https://trac.sagemath.org/ticket/3859#comment:2
<ul>
<li><strong>cc</strong>
<em>jason</em> added
</li>
<li><strong>upstream</strong>
set to <em>N/A</em>
</li>
</ul>
TicketjdemeyerTue, 13 Aug 2013 15:35:53 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:3
https://trac.sagemath.org/ticket/3859#comment:3
<ul>
<li><strong>milestone</strong>
changed from <em>sage-5.11</em> to <em>sage-5.12</em>
</li>
</ul>
Ticketvbraun_spamThu, 30 Jan 2014 21:20:52 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:4
https://trac.sagemath.org/ticket/3859#comment:4
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.1</em> to <em>sage-6.2</em>
</li>
</ul>
Ticketvbraun_spamTue, 06 May 2014 15:20:58 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:5
https://trac.sagemath.org/ticket/3859#comment:5
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.2</em> to <em>sage-6.3</em>
</li>
</ul>
Ticketvbraun_spamSun, 10 Aug 2014 16:51:03 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:6
https://trac.sagemath.org/ticket/3859#comment:6
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.3</em> to <em>sage-6.4</em>
</li>
</ul>
TicketchapotonSat, 27 Dec 2014 21:00:17 GMTstatus changed; commit, branch, author set
https://trac.sagemath.org/ticket/3859#comment:7
https://trac.sagemath.org/ticket/3859#comment:7
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
set to <em>6ff1e76a73b72c651d3a79b7c7c3f313b606372c</em>
</li>
<li><strong>branch</strong>
set to <em>u/chapoton/3859</em>
</li>
<li><strong>author</strong>
set to <em>Frédéric Chapoton</em>
</li>
</ul>
<p>
Here is a proposal, that corrects several minor wrong points in the code of <strong>Line</strong> and add more doc.
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=6ff1e76a73b72c651d3a79b7c7c3f313b606372c"><span class="icon"></span>6ff1e76</a></td><td><code>trac #3859 solving issues with Line in 3d</code>
</td></tr></table>
TicketkcrismanTue, 27 Jan 2015 02:12:04 GMTcc changed
https://trac.sagemath.org/ticket/3859#comment:8
https://trac.sagemath.org/ticket/3859#comment:8
<ul>
<li><strong>cc</strong>
<em>kcrisman</em> added
</li>
</ul>
<p>
See also <a class="ext-link" href="http://ask.sagemath.org/question/25609/how-to-find-the-full-argument-list-of-a-built-in-function/"><span class="icon"></span>http://ask.sagemath.org/question/25609/how-to-find-the-full-argument-list-of-a-built-in-function/</a> - amazingly, I have never heard of this!
</p>
TicketchapotonFri, 13 Feb 2015 21:05:55 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:9
https://trac.sagemath.org/ticket/3859#comment:9
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.4</em> to <em>sage-6.5</em>
</li>
</ul>
TicketchapotonMon, 23 Feb 2015 19:25:34 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:10
https://trac.sagemath.org/ticket/3859#comment:10
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.5</em> to <em>sage-6.6</em>
</li>
</ul>
TicketchapotonWed, 25 Mar 2015 20:32:09 GMT
https://trac.sagemath.org/ticket/3859#comment:11
https://trac.sagemath.org/ticket/3859#comment:11
<p>
Hello, review, anybody ?
</p>
TicketmmezzarobbaThu, 09 Apr 2015 08:11:17 GMT
https://trac.sagemath.org/ticket/3859#comment:12
https://trac.sagemath.org/ticket/3859#comment:12
<p>
Either I don't understand the description of what <code>corner_cutoff</code> is supposed to do, or the first example from the ticket's description still doesn't work for me:
</p>
<pre class="wiki">sage: line3d([[-1, 3, 369.26], [-1, -10.11, 125.33], [0.21, -10.13, 99.42]], corner_cutoff = 1)
</pre><p>
produces a smooth line in jmol.
</p>
TicketchapotonWed, 29 Apr 2015 12:48:34 GMTstatus, milestone changed
https://trac.sagemath.org/ticket/3859#comment:13
https://trac.sagemath.org/ticket/3859#comment:13
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>milestone</strong>
changed from <em>sage-6.6</em> to <em>sage-6.7</em>
</li>
</ul>
<p>
I guess there is still a problem.
</p>
TicketchapotonFri, 17 Jul 2015 12:05:20 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:14
https://trac.sagemath.org/ticket/3859#comment:14
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.7</em> to <em>sage-6.8</em>
</li>
</ul>
TicketgitMon, 19 Oct 2015 19:33:40 GMTcommit changed
https://trac.sagemath.org/ticket/3859#comment:15
https://trac.sagemath.org/ticket/3859#comment:15
<ul>
<li><strong>commit</strong>
changed from <em>6ff1e76a73b72c651d3a79b7c7c3f313b606372c</em> to <em>10c880c4db6e1c621c96419dbcd33e469033d2c6</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=dc911471aa22ec87fbbdd52c81d6e8220be53075"><span class="icon"></span>dc91147</a></td><td><code>Merge branch 'u/chapoton/3859' into 6.10.b0</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=10c880c4db6e1c621c96419dbcd33e469033d2c6"><span class="icon"></span>10c880c</a></td><td><code>trac #3859 fixing corner_cutoff</code>
</td></tr></table>
TicketchapotonMon, 19 Oct 2015 19:37:21 GMTstatus changed
https://trac.sagemath.org/ticket/3859#comment:16
https://trac.sagemath.org/ticket/3859#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
This should be better now:
</p>
<p>
<code>corner_cutoff = -1</code> : no smoothing
</p>
<p>
<code>corner_cutoff = 1</code> : all points smoothed
</p>
<p>
and smoothing is performed in general if the angle is close enough to pi, meaning
that successive segments are close to being aligned.
</p>
TicketchapotonMon, 19 Oct 2015 19:37:33 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:17
https://trac.sagemath.org/ticket/3859#comment:17
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.8</em> to <em>sage-6.10</em>
</li>
</ul>
TicketchapotonMon, 09 Nov 2015 16:38:19 GMT
https://trac.sagemath.org/ticket/3859#comment:18
https://trac.sagemath.org/ticket/3859#comment:18
<p>
ping ? should be an easy review !
</p>
TicketchapotonMon, 14 Dec 2015 19:24:05 GMT
https://trac.sagemath.org/ticket/3859#comment:19
https://trac.sagemath.org/ticket/3859#comment:19
<p>
PING ? nobody out there ?
</p>
TicketkcrismanWed, 16 Dec 2015 03:40:36 GMT
https://trac.sagemath.org/ticket/3859#comment:20
https://trac.sagemath.org/ticket/3859#comment:20
<p>
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.
</p>
<pre class="wiki">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.
</pre><p>
Maybe one could add parenthetically "(and hence the cosine is close to 1)" or the like as appropriate in the two places this occurs?
</p>
<pre class="wiki">+ 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)])
</pre><p>
Super-useful! I made it into an interact to test things, very nice.
</p>
<p>
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 flat-out change.
</p>
<p>
For instance, I would say that the angle is nearly <em>zero</em> 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 <em>vectors</em> formed by the two (directed) line segments that is in question.
</p>
<p>
Does that make sense?
</p>
<p>
I also have to admit that with <a class="new ticket" href="https://trac.sagemath.org/ticket/3861" title="defect: automatic line smoothing shouldn't be automatic, or should at least be ... (new)">#3861</a> that the output of
</p>
<pre class="wiki">Line([(0,0,0),(1,0,0),(2,1,0),(0,1,0)],corner_cutoff=-.5) # current ticket
</pre><p>
is bizarre and does not look like the 2d version style in any case.
</p>
<p>
Anyway, otherwise this seems to work as advertised. Should end users have access to <code>max_len</code> in <code>Line</code>? Currently I don't think that's really possible. But maybe that's okay.
</p>
TicketgitSun, 20 Dec 2015 20:49:28 GMTcommit changed
https://trac.sagemath.org/ticket/3859#comment:21
https://trac.sagemath.org/ticket/3859#comment:21
<ul>
<li><strong>commit</strong>
changed from <em>10c880c4db6e1c621c96419dbcd33e469033d2c6</em> to <em>bc5b08a9516f877b5a82f286a39f2779b8518e2d</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=41641f6be555db3c092f4baf28092f8003794f8c"><span class="icon"></span>41641f6</a></td><td><code>Merge branch 'u/chapoton/3859' into 6.10</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=bc5b08a9516f877b5a82f286a39f2779b8518e2d"><span class="icon"></span>bc5b08a</a></td><td><code>trac #3859 back to the original convention : angle 0 means going straight</code>
</td></tr></table>
TicketchapotonSun, 20 Dec 2015 20:53:34 GMT
https://trac.sagemath.org/ticket/3859#comment:22
https://trac.sagemath.org/ticket/3859#comment:22
<p>
Thanks for having a look. I know well that time is precious.
</p>
<p>
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.
</p>
<p>
Maybe this is good enough, for a 7 years old ticket ?
</p>
TicketchapotonWed, 30 Dec 2015 09:10:13 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:23
https://trac.sagemath.org/ticket/3859#comment:23
<ul>
<li><strong>milestone</strong>
changed from <em>sage-6.10</em> to <em>sage-7.0</em>
</li>
</ul>
TicketchapotonThu, 21 Jan 2016 20:21:36 GMT
https://trac.sagemath.org/ticket/3859#comment:24
https://trac.sagemath.org/ticket/3859#comment:24
<p>
<strong>*ping*</strong> ?
</p>
TicketchapotonThu, 21 Jan 2016 20:21:47 GMTmilestone changed
https://trac.sagemath.org/ticket/3859#comment:25
https://trac.sagemath.org/ticket/3859#comment:25
<ul>
<li><strong>milestone</strong>
changed from <em>sage-7.0</em> to <em>sage-7.1</em>
</li>
</ul>
TickettscrimThu, 21 Jan 2016 22:03:01 GMTreviewer set
https://trac.sagemath.org/ticket/3859#comment:26
https://trac.sagemath.org/ticket/3859#comment:26
<ul>
<li><strong>reviewer</strong>
set to <em>Karl-Dieter Crisman, Travis Scrimshaw</em>
</li>
</ul>
<p>
This LGTM, but I'd like to see if Karl-Dieter has any more comments before we set this to a positive review.
</p>
TicketkcrismanFri, 22 Jan 2016 00:56:12 GMT
https://trac.sagemath.org/ticket/3859#comment:27
https://trac.sagemath.org/ticket/3859#comment:27
<p>
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.
</p>
TickettscrimFri, 22 Jan 2016 02:30:40 GMTstatus changed
https://trac.sagemath.org/ticket/3859#comment:28
https://trac.sagemath.org/ticket/3859#comment:28
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
AFAIK it is correct. At the very least, it is better than the current behavior of erroring out.
</p>
TicketvbraunSat, 23 Jan 2016 20:42:40 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/3859#comment:29
https://trac.sagemath.org/ticket/3859#comment:29
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>u/chapoton/3859</em> to <em>bc5b08a9516f877b5a82f286a39f2779b8518e2d</em>
</li>
</ul>
Ticket