Clarify some comments concerning the matplotlib Delaunay code in list_plot3d.py

[jdemeyer]

While reading some source code to debug #12798, I found some badly worded/formatted comments.

Component: documentation

Author: Jeroen Demeyer

Reviewer: Karl-Dieter Crisman

Merged: sage-5.2.beta0

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

[jdemeyer]

Attachment: 13167_comment.patch.gz

[kcrisman]

comment:2

Looks fine on the face of it. Applies fine, tests fine, code is the same after all.

Is it really the mpl code, then, not scipy? Hang on, I just haven't looked at the whole file yet...

[kcrisman]

Reviewer: Karl-Dieter Crisman

[kcrisman]

comment:3

Ok, that's right. Good catch.

[ppurka]

comment:4

Replying to @kcrisman:

Looks fine on the face of it. Applies fine, tests fine, code is the same after all.

Is it really the mpl code, then, not scipy? Hang on, I just haven't looked at the whole file yet...

Yes. It is mpl code that is the problem here. Quoting from mpl docs,

''
If interp keyword is set to ‘nn‘ (default), uses natural neighbor interpolation based on Delaunay triangulation. By default, this algorithm is provided by the matplotlib.delaunay package, written by Robert Kern. The triangulation algorithm in this package is known to fail on some nearly pathological cases. For this reason, a separate toolkit (mpl_tookits.natgrid) has been created that provides a more robust algorithm fof triangulation and interpolation. This toolkit is based on the NCAR natgrid library, which contains code that is not redistributable under a BSD-compatible license. When installed, this function will use the mpl_toolkits.natgrid algorithm, otherwise it will use the built-in matplotlib.delaunay package.
''

So, essentially the delaunay code is not considered very robust.

Now, I had a look at the natgrid code and there is one file's license which is the main problem I think, and due to which I believe it won't be possible to distribute/include it in Sage.

[kcrisman]

comment:5

That would be a different ticket anyway.

[jdemeyer]

Merged: sage-5.2.beta0

[kcrisman]

comment:7

Apparently this has been fixed upstream. ppurka, what do you think - would that take care of it, or are there other potential issues?

[ppurka]

comment:8

Replying to @kcrisman:

Apparently this has been fixed upstream. ppurka, what do you think - would that take care of it, or are there other potential issues?

I can't remember the context for this trac ticket. Are you suggesting that we remove the code in Sage which checks for duplicate points?

[kcrisman]

comment:9

Apparently this has been fixed upstream. ppurka, what do you think - would that take care of it, or are there other potential issues?

I can't remember the context for this trac ticket. Are you suggesting that we remove the code in Sage which checks for duplicate points?

I was saying that maybe we don't need to add noise any more. But upon further review, the commit in question was about duplicate points, not points in the same subspace, so we probably still need the randomization for now. I don't know if we have code checking for duplicates.

[ppurka]

comment:10

Right. I think adding random noise is still needed. As I see it now, there are no changes necessary to the existing code.