Opened 11 years ago

Closed 4 years ago

Line's corner_cutoff is poorly documented, and buggy

Reported by: Owned by: mclean was major sage-7.1 graphics plot3d, Line, corner_cutoff, smoothing jason, kcrisman Frédéric Chapoton Karl-Dieter Crisman, Travis Scrimshaw N/A bc5b08a (Commits) bc5b08a9516f877b5a82f286a39f2779b8518e2d

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.

comment:2 Changed 10 years ago by jason

• Report Upstream set to N/A

comment:3 Changed 6 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

comment:4 Changed 6 years ago by vbraun_spam

• Milestone changed from sage-6.1 to sage-6.2

comment:5 Changed 6 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

comment:6 Changed 5 years ago by vbraun_spam

• Milestone changed from sage-6.3 to sage-6.4

comment:7 Changed 5 years ago by chapoton

• Authors set to Frédéric Chapoton
• 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:9 Changed 5 years ago by chapoton

• Milestone changed from sage-6.4 to sage-6.5

comment:10 Changed 5 years ago by chapoton

• Milestone changed from sage-6.5 to sage-6.6

comment:11 Changed 5 years ago by chapoton

Hello, review, anybody ?

comment:12 Changed 5 years ago by mmezzarobba

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 5 years ago by chapoton

• Milestone changed from sage-6.6 to sage-6.7
• Status changed from needs_review to needs_work

I guess there is still a problem.

comment:14 Changed 4 years ago by chapoton

• Milestone changed from sage-6.7 to sage-6.8

comment:15 Changed 4 years ago by git

• Commit changed from 6ff1e76a73b72c651d3a79b7c7c3f313b606372c to 10c880c4db6e1c621c96419dbcd33e469033d2c6

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

 ​dc91147 Merge branch 'u/chapoton/3859' into 6.10.b0 ​10c880c trac #3859 fixing corner_cutoff

comment:16 Changed 4 years ago by chapoton

• 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 4 years ago by chapoton

• Milestone changed from sage-6.8 to sage-6.10

comment:18 Changed 4 years ago by chapoton

ping ? should be an easy review !

comment:19 Changed 4 years ago by chapoton

PING ? nobody out there ?

comment:20 Changed 4 years ago by kcrisman

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


Super-useful! 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 flat-out 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 4 years ago by git

• Commit changed from 10c880c4db6e1c621c96419dbcd33e469033d2c6 to bc5b08a9516f877b5a82f286a39f2779b8518e2d

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

 ​41641f6 Merge branch 'u/chapoton/3859' into 6.10 ​bc5b08a trac #3859 back to the original convention : angle 0 means going straight

comment:22 Changed 4 years ago by chapoton

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 4 years ago by chapoton

• Milestone changed from sage-6.10 to sage-7.0

*ping* ?

comment:25 Changed 4 years ago by chapoton

• Milestone changed from sage-7.0 to sage-7.1

comment:26 Changed 4 years ago by tscrim

• Reviewers set to Karl-Dieter Crisman, Travis Scrimshaw

This LGTM, but I'd like to see if Karl-Dieter has any more comments before we set this to a positive review.

comment:27 Changed 4 years ago by kcrisman

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 4 years ago by tscrim

• 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 4 years ago by vbraun

• Branch changed from u/chapoton/3859 to bc5b08a9516f877b5a82f286a39f2779b8518e2d
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.