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

 
 

bug #48807: gsvd function needs matlab compatible interface

Submitted by:  Rik <rik5>
Submitted on:  Wed Aug 17 15:25:07 2016  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Matlab Compatibility
Status: ConfirmedAssigned to: None
Originator Name: Open/Closed: Open
Release: devOperating System: Any

Add a New Comment (Rich MarkupRich 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)

Mon Oct 24 20:20:29 2016, 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 Oct 5 15:24:06 2016, comment #26:

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

Guillaume <gyom>
Wed Oct 5 14:36:46 2016, comment #25:

I had to re-clone

Dmitri.

Dmitri A. Sergatskov <dasergatskov>
Wed Oct 5 10:34:48 2016, 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 Oct 4 18:20:41 2016, 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 Oct 3 19:57:58 2016, 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 Oct 3 19:29:14 2016, comment #21:

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

Rik <rik5>
Project Administrator
Mon Oct 3 18:40:15 2016, 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 Oct 2 15:40:23 2016, 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 Oct 2 15:32:10 2016, comment #18:

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

Rik <rik5>
Project Administrator
Sun Oct 2 01:11:39 2016, 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 Oct 1 22:59:16 2016, 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 Oct 1 19:46:12 2016, 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 Oct 1 19:29:10 2016, 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 Oct 1 16:29:03 2016, 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 Oct 1 16:17:20 2016, 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 Sep 28 18:50:20 2016, 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 Sep 26 21:59:24 2016, 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 Sep 26 19:11:26 2016, 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 Sep 26 18:47:14 2016, 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 Sep 11 22:33:32 2016, 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 Sep 11 20:39:12 2016, comment #6:

Can someone please run the following test cases in Matlab?

Nir Krakauer <nir_krakauer>
Sun Sep 4 16:55:14 2016, comment #5:

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

http://hg.savannah.gnu.org/hgweb/octave/rev/de24ca103c21

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

http://hg.savannah.gnu.org/hgweb/octave/rev/09005ac7d56c

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

http://hg.savannah.gnu.org/hgweb/octave/rev/0aee8b620864

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 Aug 17 17:14:21 2016, 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 Aug 17 15:49:32 2016, 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 Aug 17 15:40:36 2016, 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 Aug 17 15:31:37 2016, 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 Aug 17 15:25:07 2016, 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 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 dasergatskov (Posted a comment)
  • -unavailable- added by gyom (Posted a comment)
  • -unavailable- added by avinoam (Posted a comment)
  • -unavailable- added by bumi (Posted a comment)
  • -unavailable- added by nir_krakauer (Posted a comment)
  • -unavailable- added by carandraug (Barbara and Nir - GSOC student and mentor)
  • -unavailable- added by carandraug (Barbara and Nir - GSOC student and mentor)
  • -unavailable- added by rik5 (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
    Sun Oct 2 15:32:10 2016rik5Summarygsvd function needs BIST tests updated=>gsvd function needs matlab compatible interface
    Wed Sep 28 18:50:20 2016rik5Severity4 - Important=>3 - Normal
      StatusNone=>Confirmed
      Summarynew gsvd function is incompatible w/Matlab=>gsvd function needs BIST tests updated
    Wed Aug 17 15:31:38 2016carandraugCarbon-Copy-=>Added -unavailable-
      Carbon-Copy-=>Added -unavailable-

    Back to the top


    Powered by Savane 3.1-cleanup1