Opened 10 years ago
Last modified 8 years ago
#12798 needs_work defect
list_plot3d plots extraneous points at z=0 and doesn't take color or rgbcolor as keywords
Reported by: | Punarbasu Purkayastha | Owned by: | Jeroen Demeyer |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | graphics | Keywords: | list_plot3d, sd40.5 |
Cc: | Karl-Dieter Crisman | Merged in: | |
Authors: | Punarbasu Purkayastha, Karl-Dieter Crisman | Reviewers: | Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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.
- This function also does not take in
color
orrgbcolor
as keywords. Rest of the plot commands includingplot
,list_plot
,plot3d
do take in these keywords.
First attached patch fixes both these problems, second one fixes more doc and clarifies output.
Apply trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch, trac_12798-more-doc.3.patch and trac_12798-dont_pass_nan_to_ParametricSurface.patch to devel/sage
.
Attachments (6)
Change History (66)
comment:1 Changed 10 years ago by
Authors: | → Punarbasu Purkayastha |
---|---|
Description: | modified (diff) |
Status: | new → needs_review |
comment:2 Changed 10 years ago by
Cc: | Karl-Dieter Crisman added |
---|
comment:3 follow-up: 4 Changed 10 years ago by
Keywords: | sd40.5 added |
---|
comment:4 follow-up: 5 Changed 10 years ago by
Replying to kcrisman:
I like this idea, but have a couple questions.
- The way you have it set up currently has
color
etc. overridingtexture
. Should we be explicit about that? Is it even desirable? (Maybe it is, I just want to ask, as I am not sure.)
Actually, color
and the other keywords will never override texture
. color
will be used only when texture
is not present, and color
is present. As it is setup, if texture
is provided, then the others should be ignored. Actually, maybe we should do this explicitly and remove the other keywords if they are present. Something like
if texture == 'automatic': if 'color' in kwds: texture = kwds.pop('color') if 'rgbcolor' in kwds: del kwds['rgbcolor'] ... texture = rgbcolor(texture) if 'color' in kwds: del kwds['color'] ...
Does that sound reasonable?
- Do you have any idea what the original default
0.0
was supposed to have done in the past (say, for an empty plot)? I'm not suggesting you know, and I did read your very sensible post, but I'm just curious for any ideas you may have.
I have no idea why this was done. Was it some bug in matplotlib earlier? Was it just an oversight? For an empty plot, I suppose you would not want there to be plot points at 0.0!
comment:5 Changed 10 years ago by
Reviewers: | → Karl-Dieter Crisman |
---|---|
Status: | needs_review → needs_work |
I like this idea, but have a couple questions.
- The way you have it set up currently has
color
etc. overridingtexture
. Should we be explicit about that? Is it even desirable? (Maybe it is, I just want to ask, as I am not sure.)Actually,
color
and the other keywords will never overridetexture
.color
will be used only whentexture
is not present, andcolor
is present.
Sorry for the inaccuracy, of course that is what I meant.
As it is setup, if
texture
is provided, then the others should be ignored. Actually, maybe we should do this explicitly and remove the other keywords if they are present. Something likeif texture == 'automatic': if 'color' in kwds: texture = kwds.pop('color') if 'rgbcolor' in kwds: del kwds['rgbcolor'] ... texture = rgbcolor(texture) if 'color' in kwds: del kwds['color'] ...Does that sound reasonable?
Yes, as long as we document it. We do similar things lots of other places.
- Do you have any idea what the original default
0.0
was supposed to have done in the past (say, for an empty plot)? I'm not suggesting you know, and I did read your very sensible post, but I'm just curious for any ideas you may have.I have no idea why this was done. Was it some bug in matplotlib earlier? Was it just an oversight? For an empty plot, I suppose you would not want there to be plot points at 0.0!
Hmm, looking at your plots from the post, I think I know. The idea was probably to have the list_plot
cause a list to be plotted and then have the plot be zero elsewhere. Then there is the weird interpolation thing going on. Anyway, this makes things interpretable, though I'm not sure the original was really that useful, due to the output. I wonder who has used this in the past...
comment:7 follow-up: 8 Changed 10 years ago by
Okay, I think this idea is okay. William says to defer to what Mathematica appears to do, since this is where the function was inspired, many years ago.
There will be holes in the surface corresponding to array elements that do not represent explicit height values.
And see here, the second example - clearly not continued beyond the three points given.
But I can't get it to work.
sage: list_plot3d([(0,0,1), (2,3,4)], color='black') sage: list_plot3d([(0,0,1), (2,3,4)], color='black', rgbcolor='#0f0') sage: list_plot3d([(0,0,1), (2,3,4)], rgbcolor='#0f0') sage: list_plot3d([(0,0,1), (2,3,4)], rgbcolor='#fff') sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)], rgbcolor='#fff') sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)], rgbcolor='#00f') sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)], color='black')
None of these really seemed to look like what I wanted, or even sometimes to have anything plot. Am I using this incorrectly?
comment:8 Changed 10 years ago by
sage: list_plot3d([(0,0,1), (2,3,4)], color='black') sage: list_plot3d([(0,0,1), (2,3,4)], color='black', rgbcolor='#0f0') sage: list_plot3d([(0,0,1), (2,3,4)], rgbcolor='#0f0') sage: list_plot3d([(0,0,1), (2,3,4)], rgbcolor='#fff')
There seems to be a problem with the colors here. This is a bug that is present even without my patch. It seems like the texture is not passed onto the final function which plots the line.
sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)], rgbcolor='#fff', num_points=100) sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)], rgbcolor='#00f', num_points=100) sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)], color='black', num_points=100)
Can you check these with the additional arguments num_points=100
or higher numbers? It seems there are not enough interpolation points and so the plot is either missing or very jagged with a small value of num_points
. With the default value of f.default_value = 0.0
(as it was earlier), you do get a plot but the plot is completely incorrect and useless also.
Changed 10 years ago by
Attachment: | trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch added |
---|
Apply to devel/sage
comment:9 Changed 10 years ago by
Ok. I updated the patch to fix the color in 3D lines.
The other problem with interpolation and num_points
is something that I don't know how to fix. Giving a high value of num_points
gives a very good and accurate plot, but is also very slow. I have added some notes regarding this in the documentation of list_plot3d
.
comment:10 Changed 10 years ago by
Great, color works now - nice catch.
It turns out that the documentation for what this function actually does is insanely bad. I'm going to upload a patch clarifying what it actually does, which is not intuitive at all.
As for the issue you are seeing, I think it's very interesting. Notice that the box that comes up does have the right dimensions to incorporate all three points, even when you don't specify num_points
!
Here is what is going on - I've printed out the vals
. All those nan
guys will lead to not much of a plot.
sage: sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],interpolation_type='linear') [[ -1.00000000e+00 nan nan nan nan nan] [ nan 6.66133815e-16 nan nan nan nan] [ nan nan nan nan nan nan] [ nan nan nan 2.00000000e+00 nan nan] [ nan nan nan nan nan nan] [ nan nan nan nan nan nan]]
So you pick your poison here. Either you get something that does have the right points, but then also has everything else be extended by zero, or you don't do that, but the algorithm fails in certain cases. This will happen whenever the "box" in question is a lot bigger than the projection of the actual points to the xy-plane.
sage: sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)]) # bad sage: sage: list_plot3d([(0,5,1), (5,5,4), (-1,-5,-1)]) # good
I'm good with your work. If you like my changes, sign off as a reviewer and we'll get this in.
comment:11 Changed 10 years ago by
Authors: | Punarbasu Purkayastha → Punarbasu Purkayastha, Karl-Dieter Crisman |
---|---|
Description: | modified (diff) |
Patchbot: Apply trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch and trac_12798-more-doc.patch.
Changed 10 years ago by
Attachment: | trac_12798-more-doc.2.patch added |
---|
only added #long time to last example
comment:12 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | Karl-Dieter Crisman → Karl-Dieter Crisman, Punarbasu Purkayastha |
Your patch looks good. The documentation was indeed lacking. I added just one small comment (# long time) in the last example in your patch. Positive review from me.
comment:13 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
comment:14 Changed 10 years ago by
Merged in: | → sage-5.1.beta4 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:16 Changed 10 years ago by
This might have caused #13135.
I suppose it's possible. I don't understand why
sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],num_points=400) # long time
doesn't, then. Maybe the interpolation type? It's only
f.default_value=0.0
that seems at all conceivable as a culprit. Weird.
comment:17 Changed 10 years ago by
It seems giving interpolation type as nn for list of 3-tuples (with more than 3 points in the list) is the same as giving no interpolation type.
comment:18 Changed 10 years ago by
Merged in: | sage-5.1.beta4 |
---|---|
Milestone: | sage-5.1 → sage-5.2 |
Resolution: | fixed |
Status: | closed → new |
Unmerging due to #13135 (and the potential resource leak).
comment:19 follow-up: 20 Changed 10 years ago by
The resource leak seems to be a general problem with doctesting of 3d plots. This ticket simply exposes the defect.
comment:20 follow-up: 21 Changed 10 years ago by
Replying to ppurka:
The resource leak seems to be a general problem with doctesting of 3d plots. This ticket simply exposes the defect.
So you think that the massive slowdown is caused by the few added doctests here, not by the changes in code in this ticket? That could be tested.
comment:21 follow-up: 22 Changed 10 years ago by
Replying to jdemeyer:
Replying to ppurka:
The resource leak seems to be a general problem with doctesting of 3d plots. This ticket simply exposes the defect.
So you think that the massive slowdown is caused by the few added doctests here, not by the changes in code in this ticket? That could be tested.
Yes. It will be good to test if num_points=400
in the same doctest command without this patch still leads to the massive slowdown. I can't test it since it is apparently only visible on the solaris arch. The command that is desired to be doctested is
sage: list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],num_points=400) # long time
There is one workaround (even with the resource leak) and that is to decrease the value of num_points
in the current patch.
comment:22 Changed 10 years ago by
Replying to ppurka:
Yes. It will be good to test if
num_points=400
in the same doctest command without this patch still leads to the massive slowdown.
On Solaris SPARC, sage-5.1.beta0:
sage: time list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],num_points=400) Time: CPU 23.91 s, Wall: 34.13 s
same machine, sage-5.1.beta3 + this patch:
sage: time list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],num_points=400) Time: CPU 39.62 s, Wall: 50.75 s
So, there is a slowdown. But perhaps more importantly, the time to doctest the file list_plot3d.py
increased by much more than this. So I'm guessing with this patch, the time to doctest is much more than the sum of the times of the individual commands.
comment:23 follow-up: 24 Changed 10 years ago by
This is befuddling. The main time consuming doctest that is added is the num_points=400
one. And here are the doctest times. The starting point is sage-5.2.alpha0 with these patches added, on an Intel(R) Core(TM)2 CPU 6400 @ 2.13GHz
ubuntu 12.04.
- First, with
f.default_value = 0.0
missing from the file (this is the change introduced by the patches in this ticket)....age-5.2.alpha0/devel/sage» ../../sage -b >& /dev/null ...age-5.2.alpha0/devel/sage» ../../sage -t -long sage/plot/plot3d/list_plot3d.py sage -t -long "devel/sage-main/sage/plot/plot3d/list_plot3d.py" [13.5 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 13.5 seconds
- Second test with
f.default_value = 0.0
. Note that at this point, the extra examples introduced by the patches in this ticket are still there in the file....age-5.2.alpha0/devel/sage» ../../sage -b >& /dev/null ...age-5.2.alpha0/devel/sage» ../../sage -t -long sage/plot/plot3d/list_plot3d.py sage -t -long "devel/sage-main/sage/plot/plot3d/list_plot3d.py" [16.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 16.3 seconds
- Third, with
f.default_value = float('inf')
, and otherwise the patches in this ticket are still applied, i.e., the extra examples are still present....age-5.2.alpha0/devel/sage» ../../sage -b >& /dev/null ...age-5.2.alpha0/devel/sage» ../../sage -t -long sage/plot/plot3d/list_plot3d.py sage -t -long "devel/sage-main/sage/plot/plot3d/list_plot3d.py" [12.8 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 12.8 seconds
- Finally, with all the patches unapplied. This is vanilla sage-5.2.alpha0 and hence the examples introduced by the patches in this ticket are not present.
...age-5.2.alpha0/devel/sage» hg qpop -a popping trac_12798-more-doc.2.patch popping trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch popping trac_9774-doctests.patch popping trac_9774-mathjax-try5.patch patch queue now empty ...age-5.2.alpha0/devel/sage» ../../sage -b >& /dev/null ...age-5.2.alpha0/devel/sage» ../../sage -t -long sage/plot/plot3d/list_plot3d.py sage -t -long "devel/sage-main/sage/plot/plot3d/list_plot3d.py" [6.3 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 6.3 seconds
Note that correct plots are generated only by 1. and 3. At least on this machine, both 1. and 3. are faster than the code already present in vanilla sage-5.2.alpha0. It is only the example with num_points=400
that introduces the slowdown.
So, I am attaching an updated patch which reduces the num_points
. Here is the time taken for this new one. Notice that the time taken is lower than the vanilla sage-5.2.alpha0, even though the number of examples doctested has increased.
...age-5.2.alpha0/devel/sage» hg qpush trac_12798-more-doc.2.patch applying trac_9774-mathjax-try5.patch applying trac_9774-doctests.patch applying trac_12798-fix_extraneous_points_and_kwds_in_list_plot3d.patch applying trac_12798-more-doc.2.patch now at: trac_12798-more-doc.2.patch ...age-5.2.alpha0/devel/sage» hg qref ...age-5.2.alpha0/devel/sage» ../../sage -b >& /dev/null ...age-5.2.alpha0/devel/sage» ../../sage -t -long sage/plot/plot3d/list_plot3d.py sage -t -long "devel/sage-main/sage/plot/plot3d/list_plot3d.py" [4.4 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 4.4 seconds
comment:24 follow-up: 26 Changed 10 years ago by
comment:25 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | new → needs_review |
comment:26 Changed 10 years ago by
Replying to jdemeyer:
Replying to ppurka:
This is befuddling.
Thank you, wiktionary :-)
It was befuddling when I started writing out that comment. By the time (about 1 hour) I reached the end of that comment, I had changed my opinion and things became less muddled.
comment:27 Changed 10 years ago by
It's less bad, but far from good:
buildbot@mark:~/mark/sage-5.1.beta3$ ./sage -t devel/sage/sage/plot/plot3d/list_plot3d.py sage -t "devel/sage/sage/plot/plot3d/list_plot3d.py" [448.8 s] ---------------------------------------------------------------------- All tests passed! Total time for all tests: 449.2 seconds
Without the patch, it is something like 30 seconds.
comment:28 Changed 10 years ago by
I'm going to split the patches into a "change functionality" and "add doctests" part.
Changed 10 years ago by
Attachment: | 12798_change.patch added |
---|
Changed 10 years ago by
Attachment: | 12798_doc.patch added |
---|
comment:29 follow-up: 31 Changed 10 years ago by
I can absolutely confirm that the slowdown on Solaris SPARC is caused by 12798_change.patch, not by the added doctests.
comment:30 Changed 10 years ago by
Status: | needs_review → needs_work |
---|
comment:31 follow-up: 33 Changed 10 years ago by
Replying to jdemeyer:
I can absolutely confirm that the slowdown on Solaris SPARC is caused by 12798_change.patch, not by the added doctests.
It is confusing alright. I am getting speedups on intel but you are getting slowdowns on sparc. Sorry for bothering you so much. Can you do the following test since you have access to the sparc:
- Change line 467 and 488 of 12798_change.patch with the following version and check if it is still slow.
f.default_value = float('inf')
This is the only alternative solution I have for the incorrect plots otherwise generated.
comment:32 follow-up: 34 Changed 10 years ago by
I disagree with the analysis as 'needs work'. I understand that this is annoying, but ppurka is right that this makes something mathematically correct, or at least more so. " plots extraneous points at z=0" - we shouldn't be plotting extraneous points. And how many people are going to be plotting 3d list plots on Sparc? Very unlikely.
There probably is some Numpy internal that causes the interpolations to be very slow without a default value - at least, I wouldn't be surprised. It would be worth having someone who understands numpy (not me, unfortunately) generate a "native example".
That said, it seems like there must be some compromise that we can reach here for this ticket. Is it possible to add even more # long time
flags to get it under a minute on mark? Also note that if it's 30 seconds without the patch, that's five times as long as the six seconds above - so mark would not exactly be a "typical" machine to be talking about timings on. That's sort of like on my old PPC machine - I'm happy as long as some SAGE_TIMEOUT_LONG
value works :)
comment:33 Changed 10 years ago by
Replying to ppurka:
f.default_value = float('inf')
I'm afraid this doesn't really help (it might be just a little bit faster, but still massively slower than before).
comment:34 follow-ups: 35 38 Changed 10 years ago by
Replying to kcrisman:
Also note that if it's 30 seconds without the patch, that's five times as long as the six seconds above - so mark would not exactly be a "typical" machine to be talking about timings on.
That wasn't my point. My point was that the patch on this ticket multiplies the time (without even adding new doctests!) by a factor 12.
comment:35 Changed 10 years ago by
Also note that if it's 30 seconds without the patch, that's five times as long as the six seconds above - so mark would not exactly be a "typical" machine to be talking about timings on.
That wasn't my point. My point was that the patch on this ticket multiplies the time (without even adding new doctests!) by a factor 12.
I understand that. I suppose we have somewhat orthogonal points. My point is that we have often, in the past, lived with slowdowns if it meant correctness could be achieved. (Though in this case there may have been some good reason to have the zero points, but it was never documented so we don't know why.) Especially since there is a much smaller chance that anyone will ever use this function on a Sparc machine, and it's not actually breaking anything, it seems to be less of an issue to me. I'm really not convinced why it's important to have every file test fully in less than six minutes anyway; it seems quite arbitrary. But that is just my opinion :)
comment:36 Changed 10 years ago by
Unrelated, but while looking in this file I saw some confusing comments concerning the Delaunay call, I fixed them at #13167. Could any of you review that?
comment:38 Changed 10 years ago by
Replying to jdemeyer:
Replying to kcrisman:
Also note that if it's 30 seconds without the patch, that's five times as long as the six seconds above - so mark would not exactly be a "typical" machine to be talking about timings on.
That wasn't my point. My point was that the patch on this ticket multiplies the time (without even adding new doctests!) by a factor 12.
That's the weird part. One point of comment:23 was to establish that there is a speedup on intel cpu, though not by a large margin.
comment:39 Changed 10 years ago by
Owner: | changed from jason, was to Jeroen Demeyer |
---|
How does a doctest "plot" an object? I mean, what really happens when doctesting
sage: plot(foo)
Because the slow part is the actual plotting, not the computation of the object.
comment:40 follow-up: 42 Changed 10 years ago by
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.
comment:41 Changed 10 years ago by
I guess the method sage.plot.plot3d.parametric_surface.ParametricSurface.triangulate()
should be fixed not to add any faces of which the coordinates are not all finite numbers. However, I'm having a hard time understanding that method.
Because now we are sending a list of faces to be plotted full of NaN
s:
sage: G = list_plot3d([(0,0,1), (2,3,4), (-1,-1,-1)],num_points=100) sage: G.triangulate() sage: G.face_list() [...[(1.9696971120698907, 2.71717269474511, nan), (1.9696971120698907, 2.757576737669652, nan), (2.000000142649374, 2.757576737669652, nan), (2.000000142649374, 2.71717269474511, nan)], [(1.9696971120698907, 2.757576737669652, nan), (1.9696971120698907, 2.7979807805941936, nan), (2.000000142649374, 2.7979807805941936, nan), (2.000000142649374, 2.757576737669652, nan)], [(1.9696971120698907, 2.7979807805941936, nan), (1.9696971120698907, 2.8383848235187354, nan), (2.000000142649374, 2.8383848235187354, nan), (2.000000142649374, 2.7979807805941936, nan)], [(1.9696971120698907, 2.8383848235187354, nan), (1.9696971120698907, 2.8787888664432773, nan), (2.000000142649374, 2.8787888664432773, nan), (2.000000142649374, 2.8383848235187354, nan)], [(1.9696971120698907, 2.8787888664432773, nan), (1.9696971120698907, 2.919192909367819, nan), (2.000000142649374, 2.919192909367819, nan), (2.000000142649374, 2.8787888664432773, nan)], [(1.9696971120698907, 2.919192909367819, nan), (1.9696971120698907, 2.959596952292361, 3.9494949494949494), (2.000000142649374, 2.959596952292361, nan), (2.000000142649374, 2.919192909367819, nan)], [(1.9696971120698907, 2.959596952292361, 3.9494949494949494), (1.9696971120698907, 3.000000995216903, nan), (2.000000142649374, 3.000000995216903, nan), (2.000000142649374, 2.959596952292361, nan)]]
I think this is a true bug (and this might even have been the reason for the old default_value
).
comment:42 Changed 10 years ago by
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
.
Changed 10 years ago by
Attachment: | trac_12798-dont_pass_nan_to_ParametricSurface.patch added |
---|
apply after the earlier patches
comment:43 Changed 10 years ago by
@jdemeyer: can you test the new patch I have put up. I prevent nan
s 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
.
comment:44 Changed 10 years ago by
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 nan
s in that list.
I'm afraid this will need some more substantial work.
comment:45 follow-up: 50 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | Karl-Dieter Crisman, Punarbasu Purkayastha → Karl-Dieter Crisman, Punarbasu Purkayastha, Jeroen Demeyer |
...although, strangely enough, the doctest timeout is gone on Solaris with that patch. I'm still not happy with this patch because of the nan
s, 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.
comment:46 follow-ups: 47 48 Changed 10 years ago by
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 nan
s 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.
comment:47 Changed 10 years ago by
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
nan
s have visibly disappeared? (You know what I mean; I realize that's an oxymoron.) Maybe withnum_points=4
or something, to make it easy to see.
comment:48 follow-up: 49 Changed 10 years ago by
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
nan
s 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".
comment:49 Changed 10 years ago by
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
nan
s 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 nan
s in it, but now doesn't. So we can see that they disappeared... but I don't know how to do that.
comment:50 follow-up: 51 Changed 10 years ago by
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
nan
s, 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 nan
s 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 nan
s. 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 nan
s in the matrix vals
, and this can not be avoided if you want an accurate plot. So, it is pointless to try and remove nan
s.
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!
comment:51 Changed 10 years ago by
Replying to ppurka:
So, every time the ranges of
x
andy
do not form a square, there will benan
s in the matrixvals
, and this can not be avoided if you want an accurate plot. So, it is pointless to try and removenan
s.
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.
comment:52 follow-up: 53 Changed 10 years ago by
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.
comment:53 follow-up: 55 Changed 10 years ago by
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.
comment:54 Changed 10 years ago by
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.
comment:55 Changed 10 years ago by
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.
comment:56 Changed 9 years ago by
@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?
comment:57 Changed 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:58 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:59 Changed 8 years ago by
Milestone: | sage-6.2 → sage-6.3 |
---|
comment:60 Changed 8 years ago by
Milestone: | sage-6.3 → sage-6.4 |
---|
I like this idea, but have a couple questions.
color
etc. overridingtexture
. Should we be explicit about that? Is it even desirable? (Maybe it is, I just want to ask, as I am not sure.)0.0
was supposed to have done in the past (say, for an empty plot)? I'm not suggesting you know, and I did read your very sensible post, but I'm just curious for any ideas you may have.