bugGNU Octave - Bugs: bug #48807, gsvd function needs matlab...


bug #48807: gsvd function needs matlab compatible interface

Submitted by:  Rik <rik5>
Submitted on:  Wed 17 Aug 2016 03:25:07 PM UTC  
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Confirmed Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating 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)

Mon 24 Oct 2016 08:20:29 PM UTC, comment #27:

I connected the templated code for gsvd in liboctave to the gsvd function in libinterp/corefcn so that inputs of single type will now call the appropriate code in liboctave and return a FloatMatrix or FloatComplexMatrix (http://hg.savannah.gnu.org/hgweb/octave/rev/f1bb2f0bcfec).

I removed the warning about using matrices of single type since it is okay now, and also added BIST tests to check this.

There is still the principal problem that the outputs produced by liboctave need to be manipulated before they produce Matlab compatible results. That manipulation can either be in liboctave, or in libinterp/corefcn/gsvd.cc.

Rik <rik5>
Project Administrator
Wed 05 Oct 2016 03:24:06 PM UTC, comment #26:

Thanks, I had to run "make maintainer-clean".

Guillaume <gyom>
Wed 05 Oct 2016 02:36:46 PM UTC, comment #25:

I had to re-clone


Dmitri A. Sergatskov <dasergatskov>
Wed 05 Oct 2016 10:34:48 AM UTC, comment #24:

Compiling with the current tip (e22906126af5), I get this error:

Do I have to do something extra for a successful compilation?

Guillaume <gyom>
Tue 04 Oct 2016 06:20:41 PM UTC, comment #23:

All done. I updated the list of unimplemented and merged to stable again. I also moved gsvd in the default branch to a new section of the NEWS file.

Carnë Draug <carandraug>
Project Member
Mon 03 Oct 2016 07:57:58 PM UTC, comment #22:

@Carnë: Could you do a merge of stable on to default? I just tried to do that to pick up a bunch of patches for gnuplot, but Mercurial wanted to delete gsvd on the default branch which was confusing.

Rik <rik5>
Project Administrator
Mon 03 Oct 2016 07:29:14 PM UTC, comment #21:

No, just the unimplemented list. The spellchecker exception is so minor we can just leave it be.

Rik <rik5>
Project Administrator
Mon 03 Oct 2016 06:40:15 PM UTC, comment #20:

> In this case, we don't have gsvd in Octave or in Octave Forge anymore.

The linear-algebra package hasn't been released in more than a year. The last release still has gsvd. Still, I reverted the removal of the function http://hg.code.sf.net/p/octave/linear-algebra

> There are still bits of gsvd left on the stable branch that need to be cleaned up. Also, it should go back on to the unimplemented list.

I can put it back on the unimplemented list. About the other bits left behind, I only see the exception to the spellchecker. Are you referring to that or am I missing something else?

Carnë Draug <carandraug>
Project Member
Sun 02 Oct 2016 03:40:23 PM UTC, comment #19:

I pushed a cset on the dev branch that prepares the gsvd function for implementation. It updates the docstring to refer to the API that will be implemented, not the existing one. I added input validation for a third input, for when "economy-sized" decompositions are implemented. The code was very old in using the deprecated error_state variable so I got rid of that in favor of the new xmatrix_value call. I added FIXME notes where more work has to be done. For example, I added a warning if the user has inputs of type single. Eventually that part of the code can be deleted when the connection to the single gsvd class is made. I added a simple BIST test taken from the examples in the Matlab documentation. All of the BIST tests are now tagged with the 48807 bug number so they won't cause failures to show up in 'make check'. See http://hg.savannah.gnu.org/hgweb/octave/rev/98eeed41f372.

Rik <rik5>
Project Administrator
Sun 02 Oct 2016 03:32:10 PM UTC, comment #18:

Re-titling report back to the original issue which is that the results are not Matlab compatible.

Rik <rik5>
Project Administrator
Sun 02 Oct 2016 01:11:39 AM UTC, comment #17:

I don't think it's necessary to remove gsvd, as I stated. It's better in my opinion to have 75% of something rather than 100% of nothing. In this case, we don't have gsvd in Octave or in Octave Forge anymore.

But if you're keen to keep it in development for another cycle, okay. There are still bits of gsvd left on the stable branch that need to be cleaned up. Also, it should go back on to the unimplemented list.

Rik <rik5>
Project Administrator
Sat 01 Oct 2016 10:59:16 PM UTC, comment #16:

Yes, it is usable but this is not the right fix. I think that if we didn't had the incomplete gsvd already in the repository and someone submitted as it is, we wouldn't accept it and would ask the submitter to do it correctly without those issues. But we still don't have to accept it. gsvd has not been released in any Octave version yet so we can just revert the changes and do it properly later.

This was my mistake, I should have tested gsvd better and check Matlab compatibility before merging. If it was not in the repo, and I were to add it now to core in this state, would it really be accepted?

Carnë Draug <carandraug>
Project Member
Sat 01 Oct 2016 07:46:12 PM UTC, comment #15:

The example cases and reference results in comments #6 and #7 above can be used for updated BISTs.

Nir Krakauer <nir_krakauer>
Sat 01 Oct 2016 07:29:10 PM UTC, comment #14:

I also think like Rik that gsvd should not be removed.

The double/single issue can be treated with input validation until a proper fix will be made. But the commented out BISTs should be fixed before 4.2 release.

Avinoam Kalma <avinoam>
Project Member
Sat 01 Oct 2016 04:29:03 PM UTC, comment #13:

Why remove it? I think it's usable as is. The difference between single and double is something that could be taken care of in a bug fix release, 4.2.1. Or just add an input validation check to the m-file that emits an error if any of the inputs is single.

As for testing, the m-file approach passes the examples listed in the Matlab documentation. That's at least 4 tests.

I don't think anyone is going to start picking up the gsvd C++ code right away. In fact, I'm not sure that anybody really builds their own applications that link directly to liboctave, even though we have made it a possibility.

Rik <rik5>
Project Administrator
Sat 01 Oct 2016 04:17:20 PM UTC, comment #12:

Three issues with this m-file wrapper:

1. the function still does not work properly if matrices are of class single (it's converting to double behind the scenes and returning the double).
2. there is still no tests to check that this fix actually fixes the issue.
3. the right fix would be in gsvd c++ class. If we delay the fix for 4.4, we will then have to deal with backwards compatibility on the gsvd class.

No one has been asking for this function, we just thought it would be a quick exercise for Barbara during GSOC so I don't think there's a big issue with delaying it until release 4.4 when it's done properly.

Now that default was merged into stable, I'm revertinf the m-file change on stable, merge that to default, and then removed gsvd completely for the 4.2 release.

Carnë Draug <carandraug>
Project Member
Wed 28 Sep 2016 06:50:20 PM UTC, comment #11:

I wrote an m-file wrapper for the C++ gsvd function so that it returns Matlab-compatible results (except for ordering of singular values which I wasn't too worried about). See this cset http://hg.savannah.gnu.org/hgweb/octave/rev/73a85c6cacd1.

Ideally, we need someone to translate the m-file hack into C++ code after the 4.2.0 release. Also, I commented out all of the BIST tests since they were based on the old behavior. Those need to be updated too.

The good news, though, is that 4.2.0 release can happen with gsvd included.

Rik <rik5>
Project Administrator
Mon 26 Sep 2016 09:59:24 PM UTC, comment #10:

though if we remove gsvd, it should be restored to the linear-algebra package version intended for 4.2

Nir Krakauer <nir_krakauer>
Mon 26 Sep 2016 07:11:26 PM UTC, comment #9:

I think jwe is going to release from the development branch, and then merge dev to stable. Given that, it would be best to remove it from the development branch and then reverse that after the release.

Rik <rik5>
Project Administrator
Mon 26 Sep 2016 06:47:14 PM UTC, comment #8:

How are we dealing with this for the next release? I can push a commit that removes this code on the default branch and then revert it once 4.2 has been released. Or if the default branch is merged into stable for the release candidates, this could be undone only on the stable branch.

Carnë Draug <carandraug>
Project Member
Sun 11 Sep 2016 10:33:32 PM UTC, comment #7:

I won’t have time for it, I am overwhelmed with school.
Here is the result of Nir’s test cases in Matlab R2016a:

Barbara Lócsi <bumi>
Sun 11 Sep 2016 08:39:12 PM UTC, comment #6:

Can someone please run the following test cases in Matlab?

Nir Krakauer <nir_krakauer>
Sun 04 Sep 2016 04:55:14 PM UTC, comment #5:

I have changed the order of the arguments to match what we would expect from Matlab (and reducing code duplication)


added support for float to liboctave gsvd (but did not make an interface to it on libinterp)


and moved the class into the octave namespace as discussed some weeks ago on the mailing list


It's still needed:

  • X, C, and S must be Matlab compatible (and the tests adjusted);
  • support for class single from the Octave interpreter (with tests)

Nir or Barbara, can you do it? Without these very soon, I will remove them from the next 4.2 release.

Carnë Draug <carandraug>
Project Member
Wed 17 Aug 2016 05:14:21 PM UTC, comment #4:

It looks like there are differences between Matlab and LAPACK in how GSVD is defined. This is alluded to vaguely in both Matlab's [1] and Octave's [2] documentation.

An interface to LAPACK's GSVD routines is good to have available, regardless of what Matlab is doing.

As far as recovering Matlab's form, we can do that (up to the ordering of singular values and vectors, which seems to be consistently the opposite) for at least their Examples 1 and 2 with

[1] cf. https://groups.google.com/forum/#!topic/comp.soft-sys.matlab/aBgSZNTBcxU
[2] "Other authors, like S. Van Huffel, define this transformation as the simulatenous diagonalisation of the input matrices, this can be achieved by multiplying x by the inverse of [I 0; 0 r]."

Nir Krakauer <nir_krakauer>
Wed 17 Aug 2016 03:49:32 PM UTC, comment #3:

Check the Matlab documentation (http://www.mathworks.com/help/matlab/ref/gsvd.html). They have a description of each output, and I simply tried Example 1 that was listed there.

Rik <rik5>
Project Administrator
Wed 17 Aug 2016 03:40:36 PM UTC, comment #2:

There may be different definitions in use of the GSVD outputs. Can you post results from both Matlab and Octave for some cases, such as the examples in the documentation?

Nir Krakauer <nir_krakauer>
Wed 17 Aug 2016 03:31:37 PM UTC, comment #1:

I assumed that the function in the linear-algebra package was compatible and it was only a matter of adjusting to fit into core build system.

From the docs, it would seem that it's only the order of the output values that is incorrect. Nir and Barbara should comment on this. The place to start would be making tests for those issues with Matlab that are failing now.

Carnë Draug <carandraug>
Project Member
Wed 17 Aug 2016 03:25:07 PM UTC, original submission:

The newly introduced gsvd function in Octave has a different syntax and produces different results than Matlab (http://www.mathworks.com/help/matlab/ref/gsvd.html).

Rik <rik5>
Project Administrator


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

Attach Files:

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 dasergatskov (Posted a comment)
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by bumi (Posted a comment)
  • -email is unavailable- added by nir_krakauer (Posted a comment)
  • -email is unavailable- added by carandraug (Barbara and Nir - GSOC student and mentor)
  • -email is unavailable- added by carandraug (Barbara and Nir - GSOC student and mentor)
  • -email is unavailable- added by rik5 (Submitted the item)

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

    Only project members can vote.




    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-10-02 rik5 Summarygsvd function needs BIST tests updated => gsvd function needs matlab compatible interface
    2016-09-28 rik5 Severity4 - Important => 3 - Normal
        StatusNone => Confirmed
        Summarynew gsvd function is incompatible w/Matlab => gsvd function needs BIST tests updated
    2016-08-17 carandraug Carbon-Copy- => Added -email is unavailable-
        Carbon-Copy- => Added -email is unavailable-

    Back to the top

    Powered by Savane 3.3