Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#13167 closed enhancement (fixed)

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

Reported by: jdemeyer Owned by: mvngu
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 jdemeyer)

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 jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by jdemeyer

comment:1 Changed 9 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 follow-up: Changed 9 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_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 9 years ago by kcrisman

Ok, that's right. Good catch.

comment:4 in reply to: ↑ 2 Changed 9 years ago by ppurka

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.

Last edited 9 years ago by ppurka (previous) (diff)

comment:5 Changed 9 years ago by kcrisman

That would be a different ticket anyway.

comment:6 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.2.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:7 follow-up: Changed 9 years ago by kcrisman

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 ; follow-up: Changed 9 years ago by ppurka

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 9 years ago by 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?

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 9 years ago by ppurka

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 9 years ago by jdemeyer

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