patchGNU Octave - Patches: patch #8960, Preliminary balancing and balance...

 
 

patch #8960: Preliminary balancing and balance option for eig

Submitter:  Barbara Lócsi <bumi>
Submitted:  Tue 29 Mar 2016 10:06:49 PM UTC
   
 
Category:  Core : new feature Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 18 Jan 2020 03:52:08 PM UTC, comment #15: 

Thanks for the pointer Nir.

Closing item.

Kai Torben Ohlhus <siko1056>
Group Member
Sat 18 Jan 2020 02:37:38 PM UTC, comment #14: 

Actually, this patch has already been revised and committed to Octave: http://hg.savannah.gnu.org/hgweb/octave/rev/510886d03ef2

Nir Krakauer <nir_krakauer>
Sat 18 Jan 2020 11:00:41 AM UTC, comment #13: 

Review for Octave 6.

I think there is no progress here.  But we should really come back to this one.

Kai Torben Ohlhus <siko1056>
Group Member
Fri 29 Jul 2016 06:29:38 PM UTC, comment #12: 

It might be good to add short comments for the tests on which features are being tested.

Also, you need to expand the documentation string for eig.cc (that starts with "texinfo") to include the new calling forms.

Nir Krakauer <nir_krakauer>
Fri 29 Jul 2016 05:10:14 PM UTC, comment #11: 

I was hoping you come to IRC today too. It's nice that you removed the duplication of the code, it is much better now.

However, the tests still don't test the real thing, i.e., that the expected algorithm is used instead of another. The tests should check for that, it's the whole point of this options. Let's imagine that in a few weeks, someone makes a change on the code that ignores the setting of chol, or balance. The tests must identify that regression.

How to write tests for that? Think why would you set qz or chol? Why would you set balance or nobalance? What would be the difference in those cases?  Then write tests for that.

Also, I don't understand the purpose of this change https://bitbucket.org/BarbaraLocsi/octave/commits/2f0e9b4815d49be65a834075d64e2b7ead214b6c and your commit message doesn't mention anything.

Carnë Draug <carandraug>
Group Member
Thu 28 Jul 2016 06:25:03 PM UTC, comment #10: 

I pushed a commit[1] to my repository which reduces code duplication in the tests.

[1] https://bitbucket.org/BarbaraLocsi/octave/commits/386a37d36e45c9cd63e8f3d900d3619a2561051a

Barbara Lócsi <bumi>
Thu 28 Jul 2016 01:57:49 PM UTC, comment #9: 

The problem with F77 macros[1] were because these were recently introduced and I haven't pushed it to my repository (the patch I uploaded contained it) . The push is now done so now my repository is also up to date.

I edited eig.cc to match the coding guidelines as you noted (and deleted the unnecessary trues form the asserts)  and also pushed it to my repository.

The test you wrote as a replacement, to avoid code duplication fails for me.

%!     assert (isdiag (d))
%!     assert (isdiag (d2))
%!     assert (columns (d), 1)
%!     assert (columns (d2), 1)

(I replaced the assert (columns (d), 1) to assert (isvector(d), true) in a later commit[2] (forget to mention it in commit message) but that isn't the issue here)

In this part the test wants the d and d2 to be both diagonal matrix and a column vector at the same time, which is not what we want. We want the d and d2 to be diagonal matrix if the "matrix" flag is on and to be vector if the "vector" flag is on.

Currently my repository is up to date, it contains all of my eig related work, while the last patch I attached only miss the last commit.[3]

[1] http://hg.savannah.gnu.org/hgweb/octave/rev/59cadee1c74b
[2] https://bitbucket.org/BarbaraLocsi/octave/commits/e4462c09aa92ed5529316d368474c617f7fb9d7a
[3] https://bitbucket.org/BarbaraLocsi/octave/commits/83eb5fcafe160ce28d88b60c0805bccd9dafb531

Barbara Lócsi <bumi>
Tue 26 Jul 2016 02:16:32 PM UTC, comment #8: 

Also, just to be clear on your last attached patch? Is that a fold of all of the eig related commits on your bitbucket clone?

Carnë Draug <carandraug>
Group Member
Tue 26 Jul 2016 02:12:56 PM UTC, comment #7: 

Your many tests are also very much repetitive. Could you change them to reduce code duplication?

Note that you can create a variable that is shared between all tests with %!shared blocks, and you can also create functions that are available for the tests with %!function blocks.  But even with such blocks, there's a lot of code duplication.  For example, the following simple test could easily replace more than 300 lines of your tests (and could be extended to remove more):


%!test
%! A = [1, 2; 2, 1];
%! for output_option = {"matrix", "vector"}
%!   for balance_option = {"balance", "nobalance"}
%!     [v, d, w] = eig (A, output_option{:}, balance_option{:});
%!     [v2, d2, w2] = eig (A, balance_option{:}, output_option{:});
%!     assert (isdiag (d))
%!     assert (isdiag (d2))
%!     assert (columns (d), 1)
%!     assert (columns (d2), 1)
%!     assert (v, v2)
%!     assert (d, d2)
%!     assert (w, w2)
%!   endfor
%! endfor


Ask me on IRC if you have issues understanding %!shared and %!function in simplifying this tests.

But more important, this tests don't actually check the returned values.  Could you add tests for that?

Another note on coding guidelines, don't use brackets for one line loops and if. So instead of:


if (nargout == 0 || nargout == 1)
  {
    if (m_flag)
      {
        FloatComplexDiagMatrix d (result.eigenvalues ());
        retval = ovl (d);
      }
    else
      {
        retval = ovl (result.eigenvalues ());
      }
  }
else if (nargout == 2)
  {
    if (v_flag)
      {
        retval = ovl (result.right_eigenvectors (), result.eigenvalues ());
      }
    else
      {
        FloatComplexDiagMatrix d (result.eigenvalues ());
        retval = ovl (result.right_eigenvectors (), d);
      }
   }


You should write:


if (nargout == 0 || nargout == 1)
  {
    if (m_flag)
      retval = ovl (FloatComplexDiagMatrix (result.eigenvalues ()));
    else
      retval = ovl (result.eigenvalues ());
  }
else if (nargout == 2)
  {
    if (v_flag)
      retval = ovl (result.right_eigenvectors (), result.eigenvalues ());
    else
      retval = ovl (result.right_eigenvectors (),
                    FloatComplexDiagMatrix (result.eigenvalues ()));
   }


Carnë Draug <carandraug>
Group Member
Tue 26 Jul 2016 01:37:31 PM UTC, comment #6: 

Why did you replace F77_INT with octave_idx_type ?

Carnë Draug <carandraug>
Group Member
Sun 24 Jul 2016 10:50:03 PM UTC, comment #5: 

More people please test this, so Barbara can resolve any remaining issues over the remaining few weeks of GSoC.

Her work is also posted in a repository, https://bitbucket.org/BarbaraLocsi/octave

Nir Krakauer <nir_krakauer>
Thu 21 Jul 2016 09:23:52 AM UTC, comment #4: 

I attached a new changeset due to the conflict with:
http://hg.savannah.gnu.org/hgweb/octave/rev/59cadee1c74b

(file #37969)

Barbara Lócsi <bumi>
Mon 18 Jul 2016 10:40:48 AM UTC, comment #3: 

All of the GSoC 2016 Generalised eigenvalue problem[1][2][3] tasks are now completed, it needs to be reviewed. I attached a new changeset.
The patch contains new calling forms for eig:
preliminary balancing
computing left eigenvectors as a third output
choosing among generalized eigenvalue algorithms (chol or qz)
choosing among return value formats of the eigenvalues (vector or matrix)

[1] http://wiki.octave.org/Summer_of_Code_Project_Ideas#Generalised_eigenvalue_problem
[2] http://wiki.octave.org/User:Bumi#Project_goals
[3] http://barbaralocsigsoc2016.blogspot.com/

(file #37931)

Barbara Lócsi <bumi>
Wed 06 Jul 2016 01:28:43 PM UTC, comment #2: 

Nir and Barbara, do you think this will be ready in time for 4.2.0?  My guess is you need more time.

Note that test for built-in functions work find during "make check", and can be run individually from the build tree by typing "test path/to/file.cc-tst"  They just can't be tested by typing "test file".

Lachlan Andrew <lachlan>
Mon 09 May 2016 02:39:29 PM UTC, comment #1: 

You should have a series of test cases to make sure that the balance option (and eventually the other options you'll be adding) work as planned. Since putting tests in the source files of built-in functions is still under discussion [1], I suggest for now just writing an m-file script that runs tests for eig.

> I already know that in Matlab there is no option to change balance option in the general case, but the documentation does not mention if the balancing is default or not in the generalised case. How should I handle this?


I think balancing should be an option, but not the default, for the generalized case.

> I think that balancing a Symmetric or a Hermitian matrix does nothing, so I didn't touched those functions (so there is no balancing at all). Please correct me if I am wrong.


I believe you're right.

> I am also not sure what to do in case of an invalid string input.


Try checking what Matlab does in this case and doing likewise.

[1] http://savannah.gnu.org/bugs/?47424

Nir Krakauer <nir_krakauer>
Tue 29 Mar 2016 10:06:49 PM UTC, original submission:  

I started working on preliminary balancing for eig.

I changed the *geev function calls to the extended versions(*geevx) ( in EIG.cc and fEIG.cc) and implemented checking for 'balance' and 'nobalance' option (in eig.cc).
The *geevx LAPACK functions would allow 2 more options: permute and scale. Should these also be implemented?

I already know that in Matlab there is no option to change balance option in the general case, but the documentation does not mention if the balancing is default or not in the generalised case. How should I handle this?

I think that balancing a Symmetric or a Hermitian matrix does nothing, so I didn't touched those functions (so there is no balancing at all). Please correct me if I am wrong.

I am also not sure what to do in case of an invalid string input. I checked in some other codes and I am still not sure, in balance it does nothing if it got an invalid string as second argument, while in matrix_type it causes:
error: matrix_type: Unknown matrix type ...
So I have 2 ideas:
- error / print usage
- don't care continue with the default setting ('balance')

It is also mentioned here: bug #45944

Barbara Lócsi <bumi>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37969:  eig11.diff added by bumi (108KiB - application/octet-stream)
file #37931:  eig.diff added by bumi (182KiB - application/octet-stream)
file #36797:  pbeig.diff added by bumi (30KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by nir_krakauer (Posted a comment)
  • -email is unavailable- added by bumi (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 logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-01-18 siko1056 StatusNone Done
        Open/ClosedOpen Closed
    2020-01-18 siko1056 CategoryCore : other Core : new feature
    2020-01-18 siko1056 StatusIn Progress None
    2017-02-26 mmuetzel Dependencies- bugs #49470 is dependent
    2016-07-21 bumi Attached File- Added eig11.diff, #37969
    2016-07-18 bumi Attached File- Added eig.diff, #37931
    2016-07-07 lachlan CategoryNone Core : other
    2016-07-06 lachlan StatusNone In Progress
    2016-03-29 bumi Attached File- Added pbeig.diff, #36797

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code