bugGNU Octave - Bugs: bug #45196, gallery: raises error when calling...

 
 

bug #45196: gallery: raises error when calling randsvd, pei, kms, hanowa, gearmat.

Submitter:  Antonio Pino Robles <antoniopino>
Submitted:  Tue 26 May 2015 10:38:06 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 4.0.0 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 03 Jul 2015 04:08:49 PM UTC, comment #7: 

Most of the bugs on this bug report got fixed with the patch from bug #45466 . In addition, I have pushed the missing qmult function and several basic tests for gallery. More tests (that check more than just the size of output) would be nice. Closing as fixed.

Carnë Draug <carandraug>
Group Member
Tue 02 Jun 2015 04:51:06 PM UTC, comment #6: 

Retagging release from 4.0.0-rc4 to 4.0.0.

John W. Eaton <jwe>
Group administrator
Sat 30 May 2015 10:15:53 PM UTC, comment #5: 

Looking at the repository now, it seems like you had one commit (your first patch here that got rejected).  Then you made a new commit to backout.  Then you made two commits on top of that. You don't do it that way.

If your commit gets rejected, you update your working directory to the previous changeset and make a new commit there.  This will create a new head.  You can then either delete the other commits completely or let them be forgotten.  You should also keeo that repository up to date.  You haven't pulled any changes in the last 4 weeks (I'm guessing since you cloned it).

On the actual review of you changeset:

Please put the tests back, they are not unnecessary. But write proper tests, they are easy to write.  If, for example, you call 'gallery ("clement", 5)', it has a single outcome.  Other cases such as rando or randhess are tricker but you can still write tests for their properties.  At the very least, you could test that the returned matrix is of the right size (but please don't test this only).


Carnë Draug <carandraug>
Group Member
Sat 30 May 2015 06:44:48 PM UTC, comment #4: 

Dear Carnë,

I am sorry I didn't push the changesets. The tests I wrote were just there to try whether each function could create a 4x4 matrix of its kind, i.e. "Test for no failure".

Anyway, I though them unnecessary afterwards, I have readd them now and gallery is able to call and form those 4x4 matrices.

I reckon I have been a little messy reporting the diffs, apologies. All the changes are in octave-antonio now.

Antonio Pino Robles <antoniopino>
Sat 30 May 2015 03:00:48 PM UTC, comment #3: 

You changeset does not import cleanly. It's parent cset 7bd87990a8f40c6db26c079559d5db9dd1f2046b also does not exist on octave. You have a clone of the repository where you can push your changes http://hg.octave.org/octave-antonio but I also don't see it there.

What happened to the tests that were on the first commit? Specially on the second commit, where you change the code to qmult, you must add tests so that we know it continue to works as it is supposed to (proof of their importance is that we wouldn't be here fixing the missing qmult if I had wrote tests back then).

Carnë Draug <carandraug>
Group Member
Thu 28 May 2015 08:37:34 PM UTC, comment #2: 

Hello, I've now corrected qmult follow the guidelines, and as said vectorized the last for loop.

On the other hand the first loop it is a bit tricky: it requires to explicitly do n-1 (where n are the rows of the matrix) Householder transformations.

(file #34118, file #34119)

Antonio Pino Robles <antoniopino>
Wed 27 May 2015 04:23:28 PM UTC, comment #1: 

The test blocks seem wrong. What are they supposed to be doing?

Could you change the code in qmult to follow our coding guidelines? Brackets around conditions, space after function name and before parentheses (no space if it's indexing a variable). The commit message also needs to follow our guidelines

http://wiki.octave.org/Commit_message_guidelines

Also, instead of adding a ! to "x <= y", you could just change the operator to ">".

And in a separate commit, can the for loops in qmult be vectorized? At the least last one should be easy.

Carnë Draug <carandraug>
Group Member
Tue 26 May 2015 10:38:06 PM UTC, original submission:  

randsvd, pei, kms, hanowa, gearmat functions in gallery.m raise error when called. These were the outputs when called and the corresponding solutions.


gallery('randsvd',4)
error: 'qmult' undefined near line 2271 column 9
error: called from
    gallery>randsvd at line 2271 column 7
    gallery at line 486 column 45


Which was solved by adding the GPL'd file qmult.m, note that randsvd is part of N. J. Higham's Test Matrix Toolbox (3.0).


>> gallery('pei',4)
error: 'w' undefined near line 2018 column 24
error: called from
    gallery>pei at line 2018 column 3
    gallery at line 475 column 44
error: evaluating argument list element number 1
error: called from
    gallery>pei at line 2018 column 3
    gallery at line 475 column 44



>> gallery('kms',3)
error: 'mu' undefined near line 1608 column 23
error: called from
    gallery>kms at line 1608 column 3
    gallery at line 461 column 44
error: evaluating argument list element number 1
error: called from
    gallery>kms at line 1608 column 3
    gallery at line 461 column 4



>> gallery('hanowa',4)
error: 'lambda' undefined near line 1274 column 24
error: called from
    gallery>hanowa at line 1274 column 3
    gallery at line 453 column 44
error: evaluating argument list element number 1
error: called from
    gallery>hanowa at line 1274 column 3
    gallery at line 453 column 44
-verbatim+

The last one, 'gearmat', was trickier. Boolean operations needed rewiring:

+verbatim+
>> gallery ('gearmat',3)
error: gallery: I must be a nonzero scalar, and abs (I) <= N for gearmat matrix.
error: called from
    gallery>gearmat at line 1219 column 5
    gallery at line 450 column 44


A .diff is attached. I have also written a test (with the aid of block editing in vim) for each function in the gallery to check that the rest are OK.

Antonio Pino Robles <antoniopino>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #34118:  gallery.diff added by antoniopino (7KiB - text/x-patch)
file #34119:  vect_qmult.diff added by antoniopino (1KiB - text/x-patch)
file #34101:  gllry.diff added by antoniopino (9KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by antoniopino (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-07-03 carandraug StatusNone Fixed
        Open/ClosedOpen Closed
    2015-06-02 jwe Release4.0.0-rc4 4.0.0
    2015-05-28 antoniopino Attached File- Added gallery.diff, #34118
        Attached File- Added vect_qmult.diff, #34119
    2015-05-26 antoniopino Attached File- Added gllry.diff, #34101

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code