bugGNU Octave - Bugs: bug #47036, regressions in core m-files due to...

 
 

bug #47036: regressions in core m-files due to ismatrix changes

Submitter:  Colin Macdonald <cbm>
Submitted:  Sun 31 Jan 2016 10:52:28 PM UTC
   
 
Category:  Octave Function Severity:  4 - Important
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

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.

Rik <rik5>
Group administrator
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 ;-)

Colin Macdonald <cbm>
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.

Colin Macdonald <cbm>
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:


      if (isvector (y))
        x = 1:length (y);
      elseif (ismatrix (y))
        x = 1:rows (y);
      endif


   IMHO, that either should have an else block or a comment
   explaining.  Personally, when I think it can't happen I do:


      else
        error('Tertium non datur')
      endif


Colin Macdonald <cbm>
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.


octave:1> x = rand (48,365);
octave:2> tic; plot (x); toc
Elapsed time is 3.20617 seconds.
octave:3> y = x';
octave:4> figure (2)
octave:5> tic; plot (y); toc
Elapsed time is 0.43588 seconds.
octave:6> 3.20617 / 0.43588
ans =  7.3556


I turned on the profiler to get an idea of what is happening.


octave:7> profile on; plot (x); profile off
octave:8> profshow
   #                 Function Attr     Time (s)   Time (%)        Calls
-----------------------------------------------------------------------
  76              __go_line__             1.427      43.81          365
  70                 __line__             0.950      29.17          365
  61                  cellfun             0.346      10.63         1095
  59                 strsplit             0.076       2.32          365
  15                      get             0.075       2.31         3295
  51       __plt__>__plt2vm__             0.050       1.52            1
  21      __next_line_style__             0.043       1.32          366
  69                     line             0.033       1.03          365


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.


Rik <rik5>
Group administrator
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').

Michael Godfrey <godfrey>
Group Member
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


a = rand (48, 365);
plot (a)


It takes 10s (!) in octave and 0.1s in Matlab.  In contrast,


plot (a')


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.

Lachlan Andrew <lachlan>
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.

Colin Macdonald <cbm>
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.


tetramesh (C, X)
error: cell type invalid as index value
error: called from
    tetramesh at line 101 column 15
error: evaluating argument list element number 4
error: called from
    tetramesh at line 101 column 15


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.



Rik <rik5>
Group administrator
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.

Colin Macdonald <cbm>
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.


./statistics/tests/hotelling_test_2.m:66:  elseif (ismatrix (x))
./statistics/tests/hotelling_test.m:50:  elseif (ismatrix (x))
./plot/draw/tetramesh.m:64:  elseif (! ismatrix (X) || columns (X) != 3)
./plot/draw/tetramesh.m:62:  if (! ismatrix (T) || columns (T) != 4)
./plot/draw/shrinkfaces.m:96:  elseif (ismatrix (p) && nargin >= 2 && ismatrix (varargin{2}))
./plot/draw/private/__stem__.m:251:      elseif (ismatrix (y))
./plot/draw/private/__plt__.m:255:    elseif (ismatrix (x2))
./plot/draw/private/__plt__.m:252:  elseif (ismatrix (x1))
./plot/draw/private/__plt__.m:247:    elseif (ismatrix (x2))
./plot/draw/private/__contour__.m:78:  if (! ismatrix (z1) || ! ismatrix (x1) || ! ismatrix (y1))
./plot/draw/contourc.m:100:  if (! ismatrix (z) || ! ismatrix (x) || ! ismatrix (y))
./plot/util/private/__go_draw_axes__.m:1164:          elseif (ismatrix (xdat) && ismatrix (ydat) && ismatrix (zdat))
./plot/util/private/__go_draw_axes__.m:1157:          if (isvector (xdat) && isvector (ydat) && ismatrix (zdat))
./geometry/inpolygon.m:51:  if (! (isreal (x) && isreal (y) && ismatrix (y) && ismatrix (y)
./geometry/delaunay.m:107:        if (! ismatrix (varargin{1}) || (ncols != 2 && ncols != 3))
./geometry/delaunay.m:86:      if (! ismatrix (varargin{1})
./image/gray2ind.m:48:  elseif (! ismatrix (I) || ndims (I) < 2)
./image/rgb2hsv.m:65:  if (! ismatrix (rgb) || columns (rgb) != 3 || issparse (rgb))


Rik <rik5>
Group administrator
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.

Rik <rik5>
Group administrator
Mon 01 Feb 2016 07:16:37 AM UTC, comment #4: 

plot/draw/polar.m: looks correct as is.

Colin Macdonald <cbm>
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

Colin Macdonald <cbm>
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.

Colin Macdonald <cbm>
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,


ismatrix (x) === ( isnumeric (x) || islogical (x) || ischar (x) )


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:


gui/uigetfile.m:160:        if (ismatrix (val) && length (val) == 2)
gui/inputdlg.m:112:  elseif (ismatrix (linespec))
ode/private/ode_struct_value_check.m:125:          if ((! isnumeric (val) || ! ismatrix (val))
sparse/qmr.m:100:    elseif (isnumeric (A) && ismatrix (A))
sparse/qmr.m:127:    elseif (isnumeric (M1) && ismatrix (M1))
sparse/qmr.m:144:    elseif (isnumeric (M2) && ismatrix (M2))
sparse/bicgstab.m:79:    elseif (isnumeric(A) && ismatrix (A))
sparse/bicgstab.m:99:    elseif (isnumeric(M1) && ismatrix (M1))
sparse/bicgstab.m:111:    elseif (isnumeric(M2) && ismatrix (M2))
sparse/treeplot.m:40:  if (! ismatrix (tree) || rows (tree) != 1 || ! isnumeric (tree)
sparse/bicg.m:84:    elseif (isnumeric (A) && ismatrix (A))
sparse/bicg.m:111:    elseif (isnumeric (M1) && ismatrix (M1))
sparse/bicg.m:128:    elseif (isnumeric (M2) && ismatrix (M2))
sparse/gmres.m:82:  elseif (isnumeric (A) && ismatrix (A))
sparse/gmres.m:106:  elseif (isnumeric (M1) && ismatrix (M1))
sparse/gmres.m:118:  elseif (isnumeric (M2) && ismatrix (M2))
sparse/cgs.m:77:    elseif (isnumeric (A) && ismatrix (A))
sparse/cgs.m:97:    elseif (isnumeric (M1) && ismatrix (M1))
sparse/cgs.m:109:    elseif (isnumeric (M2) && ismatrix (M2))
sparse/spconvert.m:37:    if (nargin != 1 || ! ismatrix (m) || ! isreal (m)
optimization/fminunc.m:108:  if (nargin < 2 || nargin > 3 || ! ismatrix (x0))
optimization/lsqnonneg.m:90:      || ! (ismatrix (c) && ismatrix (d) && isstruct (options)))
optimization/fsolve.m:149:  if (nargin < 2 || nargin > 3 || ! ismatrix (x0))
optimization/pqpnonneg.m:82:      || ! (ismatrix (c) && ismatrix (d) && isstruct (options)))
image/gray2ind.m:48:  elseif (! ismatrix (I) || ndims (I) < 2)
general/cart2pol.m:52:    if (ismatrix (x) && (columns (x) == 2 || columns (x) == 3))
general/cart2pol.m:62:    if (! ((ismatrix (x) && ismatrix (y))
general/cart2pol.m:67:    if (! ((ismatrix (x) && ismatrix (y) && ismatrix (z))
general/cart2sph.m:51:    if (ismatrix (x) && columns (x) == 3)
general/cart2sph.m:59:    if (! ((ismatrix (x) && ismatrix (y) && ismatrix (z))
general/pol2cart.m:52:    if (ismatrix (theta) && (columns (theta) == 2 || columns (theta) == 3))
general/pol2cart.m:62:    if (! ((ismatrix (theta) && ismatrix (r))
general/pol2cart.m:67:    if (! ((ismatrix (theta) && ismatrix (r) && ismatrix (z))
general/sph2cart.m:51:    if (ismatrix (theta) && columns (theta) == 3)
general/sph2cart.m:59:    if (! ((ismatrix (theta) && ismatrix (phi) && ismatrix (r))
general/interp2.m:126:  if (! isnumeric (Z) || isscalar (Z) || ! ismatrix (Z) || ndims (Z) != 2)
geometry/delaunay.m:86:      if (! ismatrix (varargin{1})
geometry/delaunay.m:107:        if (! ismatrix (varargin{1}) || (ncols != 2 && ncols != 3))
geometry/inpolygon.m:51:  if (! (isreal (x) && isreal (y) && ismatrix (y) && ismatrix (y)
plot/util/private/__gnuplot_draw_axes__.m:1157:          if (isvector (xdat) && isvector (ydat) && ismatrix (zdat))
plot/util/private/__gnuplot_draw_axes__.m:1164:          elseif (ismatrix (xdat) && ismatrix (ydat) && ismatrix (zdat))
plot/draw/contourc.m:100:  if (! ismatrix (z) || ! ismatrix (x) || ! ismatrix (y))
plot/draw/private/__contour__.m:80:  if (! ismatrix (z1) || ! ismatrix (x1) || ! ismatrix (y1))
plot/draw/private/__plt__.m:247:    elseif (ismatrix (x2))
plot/draw/private/__plt__.m:252:  elseif (ismatrix (x1))
plot/draw/private/__plt__.m:255:    elseif (ismatrix (x2))
plot/draw/private/__stem__.m:251:      elseif (ismatrix (y))
plot/draw/isosurface.m:148:    if (nargin >= 3 && ismatrix (varargin{3}))
plot/draw/polar.m:218:    elseif (ismatrix (rho))
plot/draw/polar.m:235:  elseif (ismatrix (theta))
plot/draw/polar.m:251:    elseif (ismatrix (rho))
plot/draw/shrinkfaces.m:96:  elseif (ismatrix (p) && nargin >= 2 && ismatrix (varargin{2}))
plot/draw/surface.m:98:      if (isvector (x) && isvector (y) && ismatrix (z))
plot/draw/surface.m:105:      elseif (ismatrix (x) && ismatrix (y) && ismatrix (z))
plot/draw/surface.m:123:      if (isvector (x) && isvector (y) && ismatrix (z))
plot/draw/surface.m:130:      elseif (ismatrix (x) && ismatrix (y) && ismatrix (z))
plot/draw/surface.m:146:      if (ismatrix (z) && ! isvector (z) && ! isscalar (z))
plot/draw/surface.m:162:      if (ismatrix (z) && ! isvector (z) && ! isscalar (z))
plot/draw/tetramesh.m:62:  if (! ismatrix (T) || columns (T) != 4)
plot/draw/tetramesh.m:64:  elseif (! ismatrix (X) || columns (X) != 3)
statistics/tests/hotelling_test.m:50:  elseif (ismatrix (x))
statistics/tests/hotelling_test_2.m:66:  elseif (ismatrix (x))


I'm changing the title and increasing the Severity of the bug since this really does need to get solved quickly.

Rik <rik5>
Group administrator
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!

Colin Macdonald <cbm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by cbm (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-02-09 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2016-02-01 rik5 Severity3 - Normal 4 - Important
        StatusNone Confirmed
        Summarycart2sph, pol2cart et al fail for 3D arrays (b/c of is_matrix) regressions in core m-files due to ismatrix changes

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code