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

 
 

patch #8960: Preliminary balancing and balance option for eig

Submitted by:  Barbara Lócsi <bumi>
Submitted on:  Tue Mar 29 22:06:49 2016  
 
Category: Core : otherPriority: 5 - Normal
Status: In ProgressPrivacy: Public
Assigned to: NoneOpen/Closed: Open

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)

Fri Jul 29 18:29:38 2016, 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 Jul 29 17:10:14 2016, 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>
Project Member
Thu Jul 28 18:25:03 2016, 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 Jul 28 13:57:49 2016, 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.
(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 Jul 26 14:16:32 2016, 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>
Project Member
Tue Jul 26 14:12:56 2016, 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):

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:

You should write:

Carnë Draug <carandraug>
Project Member
Tue Jul 26 13:37:31 2016, comment #6:

Why did you replace F77_INT with octave_idx_type ?

Carnë Draug <carandraug>
Project Member
Sun Jul 24 22:50:03 2016, 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 Jul 21 09:23:52 2016, 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 Jul 18 10:40:48 2016, 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 Jul 6 13:28:43 2016, 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>
Project Member
Mon May 9 14:39:29 2016, 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 Mar 29 22:06:49 2016, 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 File(s):
   
   
Comment:
   

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu Jul 21 09:23:52 2016bumiAttached File-=>Added eig11.diff, #37969
    Mon Jul 18 10:40:48 2016bumiAttached File-=>Added eig.diff, #37931
    Thu Jul 7 02:03:19 2016lachlanCategoryNone=>Core : other
    Wed Jul 6 13:28:43 2016lachlanStatusNone=>In Progress
    Tue Mar 29 22:06:49 2016bumiAttached File-=>Added pbeig.diff, #36797

    Back to the top


    Powered by Savane 3.1-cleanup1