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

 
 

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

Submitted by:  Antonio Pino Robles <antoniopino>
Submitted on:  Tue 26 May 2015 10:38:06 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: 4.0.0Operating System: GNU/Linux

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)

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

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

Antonio Pino Robles <antoniopino>

 

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

Attach File(s):
   
   
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
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by antoniopino (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 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 03 Jul 2015 04:08:49 PM UTCcarandraugStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Tue 02 Jun 2015 04:51:06 PM UTCjweRelease4.0.0-rc4=>4.0.0
    Thu 28 May 2015 08:37:34 PM UTCantoniopinoAttached File-=>Added gallery.diff, #34118
      Attached File-=>Added vect_qmult.diff, #34119
    Tue 26 May 2015 10:38:06 PM UTCantoniopinoAttached File-=>Added gllry.diff, #34101

    Back to the top


    Powered by Savane 3.1-cleanup1