Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#13167 closed enhancement (fixed)

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

Reported by: Jeroen Demeyer Owned by: Minh Van Nguyen
Priority: minor Milestone: sage-5.2
Component: documentation Keywords:
Cc: Merged in: sage-5.2.beta0
Authors: Jeroen Demeyer Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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

Attachments (1)

13167_comment.patch (2.4 KB) - added by Jeroen Demeyer 10 years ago.

Download all attachments as: .zip

Change History (12)

Changed 10 years ago by Jeroen Demeyer

Attachment: 13167_comment.patch added

comment:1 Changed 10 years ago by Jeroen Demeyer

Status: newneeds_review

comment:2 Changed 10 years ago by Karl-Dieter Crisman

Reviewers: Karl-Dieter Crisman
Status: needs_reviewpositive_review

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...

comment:3 Changed 10 years ago by Karl-Dieter Crisman

Ok, that's right. Good catch.

comment:4 in reply to:  2 Changed 10 years ago by Punarbasu Purkayastha

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.

Version 1, edited 10 years ago by Punarbasu Purkayastha (previous) (next) (diff)

comment:5 Changed 10 years ago by Karl-Dieter Crisman

That would be a different ticket anyway.

comment:6 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.2.beta0
Resolution: fixed
Status: positive_reviewclosed

comment:7 Changed 10 years ago by Karl-Dieter Crisman

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

comment:8 in reply to:  7 ; Changed 10 years ago by Punarbasu Purkayastha

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?

comment:9 in reply to:  8 Changed 10 years ago by Karl-Dieter Crisman

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.

comment:10 Changed 10 years ago by Punarbasu Purkayastha

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

comment:11 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)
Note: See TracTickets for help on using tickets.