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

 
 

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

Submitted by:  Colin Macdonald <cbm>
Submitted on:  Sun 31 Jan 2016 10:52:28 PM UTC  
 
Category: Octave FunctionSeverity: 4 - Important
Priority: 5 - NormalItem Group: Regression
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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:

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

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.

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.

Rik <rik5>
Project 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>
Project 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

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.

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

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>
Project 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.

Rik <rik5>
Project 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>
Project 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,

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.

Rik <rik5>
Project 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 File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 09 Feb 2016 04:38:46 AM UTCrik5StatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Mon 01 Feb 2016 03:53:57 AM UTCrik5Severity3 - 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.1-cleanup1