Tue 09 Feb 2016 04:38:46 AM UTC, comment #16:
I applied your patches to the stable branch here: http://hg.savannah.gnu.org/hgweb/octave/rev/732ec49d1ec5. This is now all fixed up both on the stable branch, and also on the dev branch. Closing report.
|
Thu 04 Feb 2016 12:50:32 AM UTC, comment #15:
Oh, and Re: my rant about "if-elseif-end", I think x stays empty and it dies later so it probably not super important to fix anything there. It was just rant ;-)
|
Thu 04 Feb 2016 12:48:06 AM UTC, comment #14:
image/rgb2hsv.m: looks ok as is, previous lines try to massage the data into "Nx3" before it gets here. Besides, this code is all done in hg default.
|
Thu 04 Feb 2016 12:42:36 AM UTC, comment #13:
image/rgb2hsv.m: STILL TODO (perhaps only on stable branch)
plot/draw/contourc.m: ok, could have isnumeric checks added
plot/draw/tetramesh.m: ok, could have isnumeric checks added
plot/draw/shrinkfaces.m: ok, could have isnumeric checks added
geometry/delaunay.m: ok, 2D array test intended. Note 3D array inputs do in fact work (via a different code path) but (a) not supported by Matlab and (b) not documented by us, so I did not add a test for this. Nor did I prevent this feature. These other paths do not do type checking so I didn't add "isnumeric".
image/gray2ind.m: patch #8903
geometry/inpolygon.m: patch #8903
statistics/tests/hotelling_test.m: ok, 2D array test intended. No type checking to be done (other code paths don't do it)
statistics/tests/hotelling_test_2.m: ok, 2D array test intended. No type checking to be done (other code paths don't do it)
plot/util/private/__gnuplot_draw_axes__.m: ok, 2D array restriction seems intended
plot/draw/private/__plt__.m: ok, 2D array restriction is intended
plot/draw/private/__contour__.m: ok, 2D array restriction seems intended
plot/draw/private/__stem__.m: ok, 2D array restriction is intended
Re: _stem_, I dislike the "if-elseif-end" pattern:
IMHO, that either should have an else block or a comment
explaining. Personally, when I think it can't happen I do:
|
Tue 02 Feb 2016 10:40:08 PM UTC, comment #12:
@Lachlan: The plot code isn't particularly time sensitive since it tends to work on human time scales (hundreds of ms) versus CPU scales (tens of nanoseconds). But 10 seconds to plot something is way too long! At that point one is dissuaded from trying quick plots just to get an idea of what the data looks like.
In the case of upper-level files like contourc.m, the input validation is done once, not multiple times, so I don't think it is a problem.
Poor graphics performance deserves its own bug report. To check, I used your example and while the numbers are different, the gist is the same.
I turned on the profiler to get an idea of what is happening.
As you can see, there is a for loop somewhere rather than vectorized code because each of the problem routines is getting called 365 times. It looks to me like the issue is in _plt_.m where I do see for loops over the number of columns in the matrix.
It is really only two routines that make up ~3/4 of the time. _go_line_ is in C++. At some point the whole OpenGL plotting framework for Octave needs to be overhauled. The other routine is _line_ which one might be able to do something about since it is an m-file.
|
Tue 02 Feb 2016 10:24:26 PM UTC, comment #11:
For me using current dev system on Fedora 23
I get about 0.5sec for both cases (a and a').
|
Tue 02 Feb 2016 10:09:27 PM UTC, comment #10:
Rik, you say that the plot routines are not time sensitive. I disagree.
I have code that does roughly
It takes 10s (!) in octave and 0.1s in Matlab. In contrast,
is under 1s for both, so it must be something to do with the overhead per line -- much of which is validation. I've currently got it down to about 3s by doing things like validating the type once, not for each line, and will post a bug report/patch once the speed is acceptable.
In short: code in plot routines can be time sensitive.
|
Tue 02 Feb 2016 07:16:57 PM UTC, comment #9:
Ok I'll work through a few of the things on that list and send a patch tomorrow.
|
Tue 02 Feb 2016 06:55:44 PM UTC, comment #8:
Actually, the most common reason to use isnumeric is to keep out cell arrays. People often view arrays or cells as just data containers and think they should be able to use one in place of the other. And as you say, it leads to cryptic error messages.
I don't think we need to be utterly rigorous, since there is a small performance hit to always checking the inputs. If the chances are low that anyone will think to call the function with a cell array then we can skip using isnumeric in that case. I will say that the plot routines are not terrifically time sensitive so they could have the extra checking and it would be fine, but you don't need to do it if you don't feel like it.
|
Tue 02 Feb 2016 06:27:13 PM UTC, comment #7:
plot/draw/contourc.m: ok, but could have isnumeric checks added
plot/draw/tetramesh.m: ok, but could have isnumeric checks added
plot/draw/shrinkfaces.m: ok, but could have isnumeric checks added
Before I do anymore, do you think we should be adding isnumeric checks? My primary concern here is whether 3D (4D etc) arrays work where they should---what I mean by "ok" above.
The most common use of isnumeric is (I assume) to stop "keyword" or other char arguments from propagating and giving cryptic error messages. But these are not regressions per se, because old ismatrix would have allowed chars and logicals.
|
Tue 02 Feb 2016 06:11:56 PM UTC, comment #6:
@Colin: I pushed your two patches. The remaining set of instances to review is.
|
Tue 02 Feb 2016 07:03:18 AM UTC, comment #5:
I pushed a change to stable for functions uigetfile.m through pqpnonneg.m in the list (http://hg.savannah.gnu.org/hgweb/octave/rev/2935d56203a4). I will try to check in the patches you wrote tomorrow.
|
Mon 01 Feb 2016 07:16:37 AM UTC, comment #4:
plot/draw/polar.m: looks correct as is.
|
Mon 01 Feb 2016 07:01:13 AM UTC, comment #3:
plot/draw/surface.m: correct as-is
plot/draw/isosurface.m: patch #8898
general/interp2.m: patch #8898
|
Mon 01 Feb 2016 05:08:45 AM UTC, comment #2:
Ok, I submitted #8897 for cart2sph and friends using isnumeric() as you suggest. Added some tests too.
Will help with others if I can.
|
Mon 01 Feb 2016 03:53:57 AM UTC, comment #1:
I have a bad feeling that cart2sph is not the only example. I think there are probably several other scripts that were not changed when the definition of ismatrix changed for 4.0. This is because the regression tests passed so it seemed that there was nothing to change. In reality, the regression tests themselves are incomplete and are not getting 100% line coverage.
I used grep to make a list of instances of ismatrix that need to be reviewed. There are 62 instances. In the past,
Most likely ismatrix should at least be replaced by isnumeric. It probably isn't always necessary to include islogical and ischar. For exapmle, the sparse matrix m-files seem to get it right in that they check isnumeric and ismatrix since they only work on 2-D numeric matrices. The plot functions, however, probably do need to include islogical and ischar since I bet Matlab auto-converts those and plots them.
The list to review is:
I'm changing the title and increasing the Severity of the bug since this really does need to get solved quickly.
|
Sun 31 Jan 2016 10:52:28 PM UTC, original submission:
This is a regression from 3.8 because is_matrix is no longer true for 3D arrays. This means you cannot use these functions with output of "[xx,yy,zz] = meshgrid(x,y,z)".
I can make a patch for this. Would love to see it fixed for 4.0.1 but I understand if this is not realistic!
|