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.
|
Tue 02 Jun 2015 04:51:06 PM UTC, comment #6:
Retagging release from 4.0.0-rc4 to 4.0.0.
|
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).
|
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.
|
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).
|
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)
|
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.
|
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).
|