list_plot3d plots extraneous points at z=0 and doesn't take color or rgbcolor as keywords

[ppurka]

1. list_plot3d plots extra points at the horizontal x-y plane. This seems to stem from the default value of 0.0 that is assigned in the code. See this post.

2. This function also does not take in color or rgbcolor as keywords. Rest of the plot commands including plot, list_plot, plot3d do take in these keywords.

First attached patch fixes both these problems, second one fixes more doc and clarifies output.

---

Apply attachment: trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch, attachment: trac_12798-more-doc.3.patch and attachment: trac_12798-dont_pass_nan_to_ParametricSurface.patch to devel/sage.

CC: @kcrisman

Component: graphics

Keywords: list_plot3d, sd40.5

Author: Punarbasu Purkayastha, Karl-Dieter Crisman

Reviewer: Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer

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

[ppurka]

comment:42

Replying to @kcrisman:

That is a good question. You can check it out in the code, there is some stuff in graphics.py or plot.py with DOCTEST_MODE. Computing the plot objects is very fast, you are right. And plot3d of course is even different from that.

I had a look at the files. For 3D plot, the DOCTEST_MODE saves the output in every possible file format - tachyon, java3d and jmol. This is evident from the code in sage/plot/plot3d/base.pyx.

[ppurka]

apply after the earlier patches

[ppurka]

comment:43

Attachment: trac_12798-dont_pass_nan_to_ParametricSurface.patch.gz

@jdemeyer: can you test the new patch I have put up. I prevent nans from being passed on to the ParametricSurface class. Since I based it on 5.1beta5 (don't have 5.2alpha0 at home right now), you may apply the previous two patches (either ours, or yours) and then apply this new patch.

Why I opted for this - modifying ParametricSurface has a high probability of introducing more bugs than fixing the one already present. If we modify it, then one option is to put checks for nan or inf in line 588 (as of the 5.1beta5) of parametric_surface.pyx

                    res.x, res.y, res.z = self.f(uu, vv)

But doing so implies that we also need to modify urange, vrange, n and m in that file, and I believe that this requires a major editing of the code.

It seemed better/safer to actually modify list_plot3d instead of ParametricSurface.

[jdemeyer]

comment:44

That extra patch doesn't seem to have changed anything. You can check for yourself with:

sage: G = list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],num_points=100)
sage: G.triangulate()
sage: G.face_list()

There should be no nans in that list.

I'm afraid this will need some more substantial work.

[jdemeyer]

Description changed:

--- 
+++ 
@@ -5,4 +5,4 @@
 First attached patch fixes both these problems, second one fixes more doc and clarifies output.

 ---
-Apply [attachment: trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch](https://github.com/sagemath/sage-prod/files/10655277/trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch.gz) and [attachment: trac_12798-more-doc.3.patch](https://github.com/sagemath/sage-prod/files/10655279/trac_12798-more-doc.3.patch.gz) to `devel/sage`.
+Apply [attachment: trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch](https://github.com/sagemath/sage-prod/files/10655277/trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch.gz), [attachment: trac_12798-more-doc.3.patch](https://github.com/sagemath/sage-prod/files/10655279/trac_12798-more-doc.3.patch.gz) and [attachment: trac_12798-dont_pass_nan_to_ParametricSurface.patch](https://github.com/sagemath/sage-prod/files/10655282/trac_12798-dont_pass_nan_to_ParametricSurface.patch.gz) to `devel/sage`.

[jdemeyer]

Changed reviewer from Karl-Dieter Crisman, Punarbasu Purkayastha to Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer

[jdemeyer]

comment:45

...although, strangely enough, the doctest timeout is gone on Solaris with that patch. I'm still not happy with this patch because of the nans, but I don't mind if somebody wants to give it positive review.

Somebody still needs to test the patch and check whether the output 3D plots still look like they should.

[kcrisman]

comment:46

Yeah, I have to say that I'd be happier if this change was somehow doctested. Is there any place where we can document that the nans have visibly disappeared? (You know what I mean; I realize that's an oxymoron.) Maybe with num_points=4 or something, to make it easy to see.

[kcrisman]

comment:47

Replying to @kcrisman:

Yeah, I have to say that I'd be happier too if this change was somehow doctested. Is there any place where we can document that the nans have visibly disappeared? (You know what I mean; I realize that's an oxymoron.) Maybe with num_points=4 or something, to make it easy to see.

[jdemeyer]

comment:48

Replying to @kcrisman:

Yeah, I have to say that I'd be happier if this change was somehow doctested. Is there any place where we can document that the nans have visibly disappeared? (You know what I mean; I realize that's an oxymoron.)

In fact, I don't know what you mean with "visibly disappeared".

[kcrisman]

comment:49

Yeah, I have to say that I'd be happier if this change was somehow doctested. Is there any place where we can document that the nans have visibly disappeared? (You know what I mean; I realize that's an oxymoron.)

In fact, I don't know what you mean with "visibly disappeared".

Oh. What I mean is that we should be able to have a doctest that would have once output a list with some nans in it, but now doesn't. So we can see that they disappeared... but I don't know how to do that.

[ppurka]

comment:50

Replying to @jdemeyer:

...although, strangely enough, the doctest timeout is gone on Solaris with that patch. I'm still not happy with this patch because of the nans, but I don't mind if somebody wants to give it positive review.

Somebody still needs to test the patch and check whether the output 3D plots still look like they should.

I had a fresh look at this ticket. The last patch that I posted which trims the xvals and yvals on the basis of whether there are nans or not, is actually incorrect. This can be tested on a simple 3D plot:

list_plot3d([(0,0,1), (1,0,0), (0,1,0)], num_points=5)

The output is a square instead of a triangle and the square persists even for num_points=100. This is because we trim the x and y values. Without the third patch, the earlier patches in this ticket actually gives a correct plot (even for such a low value of num_points=5).

As for the presence of nans. This is all correct and there is no contradiction or error here. If you look at what is going on, there is a matrix being created called vals which contains all possible z values corresponding to x in xvals and y in yvals which should appear in our 3D plot. In the simple example above, xvals = [0, 0.25, 0.5, 0.75, 1] and yvals is the same. So all possible points (x,y) with x and y taking values from xvals and yvals, respectively, is the cartesian product xvals x yvals and it will will include points like (0.75, 0.75) or (0.75, 1), etc, which should have no corresponding finite value in vals. All these values in the matrix are set to nan. Prior to this ticket, they were being set to 0.

So, every time the ranges of x and y do not form a square, there will be nans in the matrix vals, and this can not be avoided if you want an accurate plot. So, it is pointless to try and remove nans.

So now, the question is, why does ParametricSurface take so much time only during the doctests and only on Solaris arch. I don't know the answer, and the code in that class (and in IndexFace, etc) is too complex for me and not very well documented.

At this point, what I suggest is to merge the changes regarding the colors, and leave the list_plot3d alone. If it sounds reasonable, then I will open a new ticket regarding the colors and leave this ticket for the future, when the Solaris arch goes out of fashion!

[jdemeyer]

comment:51

Replying to @ppurka:

So, every time the ranges of x and y do not form a square, there will be nans in the matrix vals, and this can not be avoided if you want an accurate plot. So, it is pointless to try and remove nans.

I completely disagree with this. At the end, you're drawing an arbitrary 3D figure given by faces and vertices. This doesn't have to lie over a square grid. Of course, currently the functions are written to require a square grid, but there is no fundamental reason for this. I agree it's not trivial to change this, but don't blame Solaris.

[vbraun]

comment:52

Nan is the standard to denote missing floating point values and should be used. It is IEEE standard and perfectly portable across all architectures where you can reasonably expect to get an accurate answer for floating point operations. Having said that, the special IEEE floating point values (nan, inf, subnormal) have sometimes been extremely slow on ancient processor architectures. For example, see http://www.sonic.net/~jddarcy/Research/fleckmrk.pdf tests an older ultrasparc and finds that nan and inf are fine, but subnormal can be more than three orders of magnitude slower.

On the plus side, modern processors don't really suffer from these childhood diseases any more and it is generally preferable to use IEEE propagation of nans through floating point operations than riddle your code with if/else and the ensuing pipeline stalls. So if the only problem with this ticket is that SPARC is slow then lets just reduce the number of points (doesn't really add anything new to the doctest anyways) and ship it.

[jdemeyer]

comment:53

Replying to @vbraun:

On the plus side, modern processors don't really suffer from these childhood diseases any more and it is generally preferable to use IEEE propagation of nans through floating point operations than riddle your code with if/else and the ensuing pipeline stalls.

Seriously, you're arguing that rendering 3D figures with NaN coordinates is a good thing? I find it hard to believe that rendering a figure with 2000 faces (half of which aren't drawn due to NaNs) is faster than first removing the offending faces and rendering a figure with 1000 faces.

[vbraun]

comment:54

If most of the points are invalid then you should of course use a different base grid, but I think thats not the scope of this ticket. The way I understand it, this ticket is about a fast rectangular base grid that doesn't fall flat on its face if there is a pole somewhere.

[ppurka]

comment:55

Replying to @jdemeyer:

Replying to @vbraun: Seriously, you're arguing that rendering 3D figures with NaN coordinates is a good thing? I find it hard to believe that rendering a figure with 2000 faces (half of which aren't drawn due to NaNs) is faster than first removing the offending faces and rendering a figure with 1000 faces.

I haven't looked at if it is Sage which is doing the rendering. But if Sage is trying to actually plot/render the points - the nans are specific 3D points (x,y,nan), not entire faces - then it is doing something very inefficient. What I would expect any reasonable plotting routine to do is simply ignore all the points which are not finite real numbers while rendering the plot. This is no different than the procedure of looping through the points and discarding them before providing them to the same plotting routine. Maybe you can have special algorithms for quickly discarding "faces" or larger collections of adjacent points, while generating a set of interpolated points. But this is useful if we control all aspects of 3D plotting in Sage itself.

Note that we are not generating the grid - the grid is being provided by mpl after interpolation. Anyway, the slowdown is architecture specific (there's speedups on intel, as I mentioned before) and triggered only on doctest mode; and if you would like it to stay that way then so be it. Only reasonable way to fix list_plot3d if you don't want to allow nans is to rewrite half of the 3D plotting methods (ParametricSurface), but I don't have the time or inclination to do this now.

[ppurka]

comment:56

@jdemeyer The doctest system has changed since this ticket was opened. So can you apply trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch again and see if it still causes timeouts on solaris?