Re: Code integration into Octave core of ilu and ichol functions.

[Kai Torben Ohlhus]

On Wed, Aug 13, 2014 at 12:29 AM, Eduardo <address@hidden> wrote:

2014-08-12 23:09 GMT+01:00 Kai Torben Ohlhus <address@hidden>:

On Tue, Aug 12, 2014 at 6:45 PM, Eduardo <address@hidden> wrote:

Hi Kai,

I have moved the code from my personal repository to a new Octave subrepo I have set on bitbucket. I made the changes necessary for adapting the code to compile within Octave development branch. The locations of the files in the source code tree  are:

(1) libinterp/dldfcn for ilu0.cc iluc.cc ilutp.cc ichol0.cc and icholt.cc files (dld functions)

(2) scripts/sparse for ilu.m and iluc.m

I chose those places because I saw (looking at your repository) you did last year this way. The new subrepo can be accessed from here [1]. 

I also have some questions about the way you managed the visibility of dld functions in terms of being able or not to call them from the interpreter. I noticed that your approach last year was to let the functions in location (1) be callable from the interpreter, maybe because they offered some extra functionality that was not used by the wrapper functions (ilu.m and ichol.m). In my case they do not.  So maybe a good idea would be to make them in some way private so they can be accessed by the wrappers ilu.m and ichol.m but not callable from the interpreter. Is this the way to go? If so, how can this be done?

[1] https://address@hidden/edu159/octave-edu159

Regards,

Eduardo

Hi Eduardo,

Regarding your question, you're right. I chose to let the individual functions to be callable to someone who has a deeper knowledge about the factorizations and wants to directly apply them. However, when I take a look in the folder dldfcn, the current convection seems to be creating a __filename__.cc file for internal functions that are not intended to be used. But they are not hidden then that is for sure. I don't know if a "private" subdirectory makes sense in this case, like it is done for m-Files.

Best,

Kai

I have taken a look to __foo__.cc kind of functions and you are right, they are not hidden and can be called from the interpreter. That is a bit ugly IMO. Anyway, any of the maintainers knows which would be the best way to go?  

Thanks in advance.

Eduardo.

Hi Eduardo,

I had a closer look to your code and here is my first good impression:

octave:15> test ilu

PASSES 6 out of 6 tests

octave:16> test ilu0

PASSES 21 out of 21 tests

octave:17> test iluc

PASSES 24 out of 24 tests

octave:18> test ilutp

PASSES 31 out of 31 tests

octave:19> test ichol

PASSES 21 out of 21 tests

octave:20> test ichol0

PASSES 17 out of 17 tests

octave:21> test icholt

PASSES 26 out of 26 tests

Congratulations, I think your code is ready to get into the main development repository!

But nevertheless, there are some things to improve:

1) In ilutp.cc there are some test cases for iluc

2) Check the formatting in all files (maybe use tools like http://astyle.sourceforge.net/ or https://www.gnu.org/software/indent/ for this work)

3) There are a few typos in the comments

Regarding the big issue with the hiding of sub-functions like ilu0.cc, simply take for now the __filename__.cc pattern, until we hear of a better idea. Then you can proceed to your final step, the Octave Changeset. Therefore read these carefully (again):

http://wiki.octave.org/Commit_message_guidelines

http://wiki.octave.org/Mercurial

Finally submit your work as a Changeset to the patch tracker.

https://savannah.gnu.org/patch/?group=octave

Until the end of GSoC you can still try to improve the documentation and the test cases, what is the intention of the soft pencils down. But the majority of you work is done I think.

Best,

Kai