patchGNU Octave - Patches: patch #10278, GSoC 2022: add support for sparse...

 
 

patch #10278: GSoC 2022: add support for sparse computations in ode15{i,s} using Octave classes , making dependency on KLU optional.

Submitter:  Sarrah Bastawala <sarrah>
Submitted:  Mon 12 Sep 2022 04:04:12 PM UTC
   
 
Category:  Core : other Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 22 Mar 2023 05:46:16 PM UTC, comment #31: 

comment #28:

> What is the exact error with the changes from comment #24 and comment #26? Could you show the exact changes that you applied (as a patch or `verbatim` block)? Could you please attach the generated file "libinterp/dldfcn/module.mk" (in your source tree) after you applied those changes?



The reason for the problem arising last time was probably something different, there was some cache which was preventing installation of a package and causing some error.

I tried again with the following changes as suggested by you and it worked. Thanks a lot !


diff --git a/libinterp/dldfcn/config-module.awk b/libinterp/dldfcn/config-module.awk
index 1a2ea9cc8b..0ab680ef4c 100644
--- a/libinterp/dldfcn/config-module.awk
+++ b/libinterp/dldfcn/config-module.awk
@@ -22,11 +22,11 @@
 ## <https://www.gnu.org/licenses/>.
 ##
 ########################################################################
-
+
 BEGIN {
   FS = "|";
   nfiles = 0;
-
+
   print "## DO NOT EDIT -- generated from module-files by config-module.awk";
   print ""
   print "EXTRA_DIST += \\"
@@ -45,15 +45,14 @@ BEGIN {
   libraries[nfiles] = $4;
 } END {
   sep = " \\\n";
-  print "DLDFCN_SRC = \\";
+  printf ("DLDFCN_SRC =");
   for (i = 1; i <= nfiles; i++) {
-    if (i == nfiles)
-      sep = "\n";
-    printf ("  %%reldir%%/%s%s", files[i], sep);
+    split (files[i], src_files, " ");
+    printf ("%s  %%reldir%%/%s", sep, src_files[1]);
   }
   print "";
-
-  sep = " \\\n";
+
+  print "";
   print "DLDFCN_LIBS = $(DLDFCN_SRC:.cc=.la)";
   print "";
   print "octlib_LTLIBRARIES += $(DLDFCN_LIBS)";
@@ -63,21 +62,16 @@ BEGIN {
   print "";
   print "%.oct : %.la"
   print "\t$(AM_V_GEN)$(INSTALL_PROGRAM) %reldir%/.libs/$(shell $(SED) -n -e \"s/dlname='\\([^']*\\)'/\\1/p\" < $<) $@"
-
+
   for (i = 1; i <= nfiles; i++) {
-    basename = files[i];
+    split (files[i], src_files, " ");
+    basename = src_files[1];
     sub (/\.cc$/, "", basename);
     print "";
-    if (files[i] == "__ode15__.cc")
-    {
-      printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/__ode15__.cc %%reldir%%/oct-sundials.cc \n",
-            basename);
-    }
-    else
-    {
-      printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/%s\n",
-            basename, files[i]);
-    }
+    printf ("%%canon_reldir%%_%s_la_SOURCES =", basename);
+    for (j in src_files)
+      printf (" %%reldir%%/%s", src_files[j]);
+    print "";
     if (cppflags[i])
       {
         printf ("%%canon_reldir%%_%s_la_CPPFLAGS = $(libinterp_liboctinterp_la_CPPFLAGS) %s\n",
@@ -90,15 +84,21 @@ BEGIN {
     printf ("%%canon_reldir%%_%s_la_DEPENDENCIES = $(OCT_LINK_DEPS)\n",
             basename);
   }
-
+
   print "";
   print "$(srcdir)/%reldir%/module.mk: $(srcdir)/%reldir%/config-module.sh $(srcdir)/%reldir%/config-module.awk $(srcdir)/%reldir%/module-files";
   print "\t$(AM_V_GEN)$(SHELL) $(srcdir)/%reldir%/config-module.sh $(srcdir)";
-
+
   print "";
   print "DLDFCN_OCT_FILES = $(DLDFCN_LIBS:.la=.oct)";
   print "";
-  print "DLDFCN_DEFUN_FILES = $(DLDFCN_SRC)";
+  printf ("DLDFCN_DEFUN_FILES =");
+  for (i = 1; i <= nfiles; i++) {
+    split (files[i], src_files, " ");
+    for (j in src_files)
+      printf ("%s  %%reldir%%/%s", sep, src_files[j]);
+  }
+  print "";
   print "";
   print "DLDFCN_PKG_ADD_FILE = %reldir%/PKG_ADD";
   print "";
@@ -123,7 +123,7 @@ BEGIN {
   print " $(DLDFCN_OCT_FILES)";
   print "";
   print "DIRSTAMP_FILES += %reldir%/$(octave_dirstamp)";
-
+
   print "";
   print "libinterp_CLEANFILES += \\";
   print "  $(DLDFCN_PKG_ADD_FILE) \\";
diff --git a/libinterp/dldfcn/module-files b/libinterp/dldfcn/module-files
index da6589c09f..b21ac6f7c0 100644
--- a/libinterp/dldfcn/module-files
+++ b/libinterp/dldfcn/module-files
@@ -4,7 +4,7 @@ __fltk_uigetfile__.cc|$(FLTK_CPPFLAGS) $(FT2_CPPFLAGS)|$(FLTK_LDFLAGS) $(FT2_LDF
 __glpk__.cc|$(GLPK_CPPFLAGS)|$(GLPK_LDFLAGS)|$(GLPK_LIBS)
 __init_fltk__.cc|$(FLTK_CPPFLAGS) $(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)|$(FLTK_LDFLAGS) $(FT2_LDFLAGS)|$(FLTK_LIBS) $(FT2_LIBS) $(OPENGL_LIBS)
 __init_gnuplot__.cc|$(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)||
-__ode15__.cc|$(SUNDIALS_XCPPFLAGS)|$(SUNDIALS_XLDFLAGS)|$(SUNDIALS_XLIBS)
+__ode15__.cc oct-sundials.cc|$(SUNDIALS_XCPPFLAGS)|$(SUNDIALS_XLDFLAGS)|$(SUNDIALS_XLIBS)
 __voronoi__.cc|$(QHULL_CPPFLAGS)|$(QHULL_LDFLAGS)|$(QHULL_LIBS)
 audiodevinfo.cc|$(PORTAUDIO_CPPFLAGS)|$(PORTAUDIO_LDFLAGS)|$(PORTAUDIO_LIBS)
 audioread.cc|$(SNDFILE_CPPFLAGS)|$(SNDFILE_LDFLAGS)|$(SNDFILE_LIBS)


Sarrah Bastawala <sarrah>
Wed 22 Mar 2023 05:34:14 PM UTC, comment #30: 


comment #29:

> I added some comments to that commit on GitHub. (Actually twice because the first round of comments disappeared into somewhere I can't find...)


Apologies for the late reply, I had my university exams going on.
I have replied to the comments made and fixed most of them in the upcoming commits

Sarrah Bastawala <sarrah>
Fri 10 Mar 2023 02:04:03 PM UTC, comment #29: 

I added some comments to that commit on GitHub. (Actually twice because the first round of comments disappeared into somewhere I can't find...)

Markus Mützel <mmuetzel>
Group administrator
Fri 10 Mar 2023 07:12:16 AM UTC, comment #28: 

What is the exact error with the changes from comment #24 and comment #26? Could you show the exact changes that you applied (as a patch or `verbatim` block)? Could you please attach the generated file "libinterp/dldfcn/module.mk" (in your source tree) after you applied those changes?

Markus Mützel <mmuetzel>
Group administrator
Thu 09 Mar 2023 07:03:13 PM UTC, comment #27: 

Hello @mmuetzel, thank you for all your comments.

As the rebase caused quite some changes, as changes to the coding style were made to the `__ode15__.cc` file which conflicted with my own changes, it took me some time and a lot of commits to figure that out. Since the current changes were spread over multiple commits, and seeing your comments you too had some difficulty understanding how they were structured, I have fixed all the whitespace and unwanted changes and squashed all my commits into one with a clean code diff for you to review, situated at
https://github.com/sarrah-basta/octave/tree/new-gsoc-final-aptch .

This contains all changes except the following:

comment #26:

> These changes to `config-module.awk` will hopefully take care of the new file :


I tried testing this, and while it kind of works, meaning the code for the implementation I have written, containing Octave Wrappers for IDA, works correctly, i.e the oct-sundials.* files are being compiled and used correctly, it runs into an error related to SUNContext (defined as a configure flag) in the KLU part (which was supprted prior to my project). I will try figuring out the exact problem and fix the script so that the files are not hardcoded soon.

Sarrah Bastawala <sarrah>
Tue 07 Mar 2023 03:43:48 PM UTC, comment #26: 

These changes to `config-module.awk` will hopefully take care of including the new file in the tarball:

diff -r 96ed8d1d0c59 libinterp/dldfcn/config-module.awk
--- a/libinterp/dldfcn/config-module.awk        Sat Mar 04 00:59:48 2023 -0500
+++ b/libinterp/dldfcn/config-module.awk        Tue Mar 07 16:42:35 2023 +0100
@@ -45,15 +45,14 @@
   libraries[nfiles] = $4;
 } END {
   sep = " \\\n";
-  print "DLDFCN_SRC = \\";
+  printf ("DLDFCN_SRC =");
   for (i = 1; i <= nfiles; i++) {
-    if (i == nfiles)
-      sep = "\n";
-    printf ("  %%reldir%%/%s%s", files[i], sep);
+    split (files[i], src_files, " ");
+    printf ("%s  %%reldir%%/%s", sep, src_files[1]);
   }
   print "";
+  print "";

-  sep = " \\\n";
   print "DLDFCN_LIBS = $(DLDFCN_SRC:.cc=.la)";
   print "";
   print "octlib_LTLIBRARIES += $(DLDFCN_LIBS)";
@@ -65,11 +64,14 @@
   print "\t$(AM_V_GEN)$(INSTALL_PROGRAM) %reldir%/.libs/$(shell $(SED) -n -e \"s/dlname='\\([^']*\\)'/\\1/p\" < $<) $@"

   for (i = 1; i <= nfiles; i++) {
-    basename = files[i];
+    split (files[i], src_files, " ");
+    basename = src_files[1];
     sub (/\.cc$/, "", basename);
     print "";
-    printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/%s\n",
-            basename, files[i]);
+    printf ("%%canon_reldir%%_%s_la_SOURCES =", basename);
+    for (j in src_files)
+      printf (" %%reldir%%/%s", src_files[j]);
+    print "";
     if (cppflags[i])
       {
         printf ("%%canon_reldir%%_%s_la_CPPFLAGS = $(libinterp_liboctinterp_la_CPPFLAGS) %s\n",
@@ -90,7 +92,13 @@
   print "";
   print "DLDFCN_OCT_FILES = $(DLDFCN_LIBS:.la=.oct)";
   print "";
-  print "DLDFCN_DEFUN_FILES = $(DLDFCN_SRC)";
+  printf ("DLDFCN_DEFUN_FILES =");
+  for (i = 1; i <= nfiles; i++) {
+    split (files[i], src_files, " ");
+    for (j in src_files)
+      printf ("%s  %%reldir%%/%s", sep, src_files[j]);
+  }
+  print "";
   print "";
   print "DLDFCN_PKG_ADD_FILE = %reldir%/PKG_ADD";
   print "";


Still untested.

Markus Mützel <mmuetzel>
Group administrator
Tue 07 Mar 2023 03:15:48 PM UTC, comment #25: 

Try using this alternative change additionally to the change from comment #24:

diff -r 96ed8d1d0c59 libinterp/dldfcn/config-module.awk
--- a/libinterp/dldfcn/config-module.awk        Sat Mar 04 00:59:48 2023 -0500
+++ b/libinterp/dldfcn/config-module.awk        Tue Mar 07 16:12:21 2023 +0100
@@ -49,7 +49,8 @@
   for (i = 1; i <= nfiles; i++) {
     if (i == nfiles)
       sep = "\n";
-    printf ("  %%reldir%%/%s%s", files[i], sep);
+    split (files[i], src_files, " ");
+    printf ("  %%reldir%%/%s%s", src_files[1], sep);
   }
   print "";

@@ -65,11 +66,14 @@
   print "\t$(AM_V_GEN)$(INSTALL_PROGRAM) %reldir%/.libs/$(shell $(SED) -n -e \"s/dlname='\\([^']*\\)'/\\1/p\" < $<) $@"

   for (i = 1; i <= nfiles; i++) {
-    basename = files[i];
+    split (files[i], src_files, " ");
+    basename = src_files[1];
     sub (/\.cc$/, "", basename);
     print "";
-    printf ("%%canon_reldir%%_%s_la_SOURCES = %%reldir%%/%s\n",
-            basename, files[i]);
+    printf ("%%canon_reldir%%_%s_la_SOURCES =", basename);
+    for (j in src_files)
+      printf (" %%reldir%%/%s", src_files[j]);
+    print "";
     if (cppflags[i])
       {
         printf ("%%canon_reldir%%_%s_la_CPPFLAGS = $(libinterp_liboctinterp_la_CPPFLAGS) %s\n",


It is completely untested. So, try to build with it before using it.

There might still be an issue with the new file missing from the distribution tarball. But I hope, it'd do the same that your hardcoded exception currently does.

Markus Mützel <mmuetzel>
Group administrator
Tue 07 Mar 2023 12:27:02 PM UTC, comment #24: 

Keeping the implementation in two separate files is probably ok.
In that case, it is probably also ok to have a separate header in which the new functions are declared.

But instead of special casing in `config-module.awk`, could you try the following change?

diff -r 96ed8d1d0c59 libinterp/dldfcn/module-files
--- a/libinterp/dldfcn/module-files        Sat Mar 04 00:59:48 2023 -0500
+++ b/libinterp/dldfcn/module-files        Tue Mar 07 13:25:39 2023 +0100
@@ -4,7 +4,7 @@
 __glpk__.cc|$(GLPK_CPPFLAGS)|$(GLPK_LDFLAGS)|$(GLPK_LIBS)
 __init_fltk__.cc|$(FLTK_CPPFLAGS) $(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)|$(FLTK_LDFLAGS) $(FT2_LDFLAGS)|$(FLTK_LIBS) $(FT2_LIBS) $(OPENGL_LIBS)
 __init_gnuplot__.cc|$(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)||
-__ode15__.cc|$(SUNDIALS_XCPPFLAGS)|$(SUNDIALS_XLDFLAGS)|$(SUNDIALS_XLIBS)
+__ode15__.cc oct-sundials.cc|$(SUNDIALS_XCPPFLAGS)|$(SUNDIALS_XLDFLAGS)|$(SUNDIALS_XLIBS)
 __voronoi__.cc|$(QHULL_CPPFLAGS)|$(QHULL_LDFLAGS)|$(QHULL_LIBS)
 audiodevinfo.cc|$(PORTAUDIO_CPPFLAGS)|$(PORTAUDIO_LDFLAGS)|$(PORTAUDIO_LIBS)
 audioread.cc|$(SNDFILE_CPPFLAGS)|$(SNDFILE_LDFLAGS)|$(SNDFILE_LIBS)


That might still need some tinkering in `config-module.awk`. But it would be a more general solution if we'd get that to work.

Markus Mützel <mmuetzel>
Group administrator
Tue 07 Mar 2023 06:56:39 AM UTC, comment #23: 

I added a couple of comments on GitHub.

Something seems to have gone wrong when merging the whitespace changes in `__ode15__.cc`. It's difficult to review the changes in that file in it's current state.
The code between `OCTAVE_NAMESPACE_*` blocks is no longer indented. Could you please adapt for that style change?

Markus Mützel <mmuetzel>
Group administrator
Tue 07 Mar 2023 06:25:33 AM UTC, comment #22: 

comment #21:

> comment #20:

> I am currently facing a slight issue in the operating system I work in, but I'll make all these changes at once and commit soon.


Hello @mmuetzel and @cdf, thank you for waiting. I have made all the suggested style changes and rebased the current code on the current head of the default branch, and fixed the slight errors caused due to the same (I suppose the macro for declaring the Octave namespace has changed).

The now working and able to merge with gnu-octave/octave:default branch is now located at https://github.com/sarrah-basta/octave/tree/gsoc-submission-final .

I could squash the recent commits into 1 final if necessary to make it easier to review.



comment #18:

> All other .oct libraries in Octave are defined in one single .cc file (+ headers). Would it make sense to merge your changes into `__ode15__.cc`?
> Additionally, the .oct file libraries are not meant to be linked to by other projects. They are rather `dlopen`ed by Octave on runtime. Do we really need a header with the declarations in that case?
>


@mmuetzel, this is the only suggested change I have not yet implemented. I suppose the current structure once again mimics so -called "implementations" in the SUNDIALS API and I had hence placed them in seperate files to be pre-compiled. From Octave's perspective though, I believe the functions in the header files are just helper functions to the `__ode15__` function, and hence could be placed in the `__ode15__.cc` file itself, as the SUNDIALS API is calling them via pointers instead of function declarations. I will still test this once on a seperate branch and link the commit here once it works

Sarrah Bastawala <sarrah>
Sat 04 Mar 2023 12:58:49 PM UTC, comment #21: 

comment #20:

> Another style remark:
> You often use the pattern `return(val);` or `return (val);` when returning a value from a function. For Octave code, you should use `return val;` (with a single space and no parenthesis).


Alright, I'll fix these soon too.
 
I am currently facing a slight issue in the operating system I work in, but I'll make all these changes at once and commit soon.

Sarrah Bastawala <sarrah>
Sat 04 Mar 2023 12:43:13 PM UTC, comment #20: 

Another style remark:
You often use the pattern `return(val);` or `return (val);` when returning a value from a function. For Octave code, you should use `return val;` (with a single space and no parenthesis).

Markus Mützel <mmuetzel>
Group administrator
Fri 03 Mar 2023 03:44:11 PM UTC, comment #19: 


comment #17:

> IIUC, you could use the exact same names as the one in SUNDIALS if you were to move those functions to the octave namespace. The one from SUNDIALS could be called as `::example_function`. And the one from Octave could be called as `octave::example_function`. Those would be different identifiers.
>


I think when something similar had come up during talking to Carlo as well, and some of the functions have their names based on SUNDIALS native types. And iirc, the consensus then was we shouldn't have something called a SUNSparseMatrix() as a function in Octave. While some of them don't have this and can be called by their exact name as you said example octave::N_VScaleAddMulti ...

Sarrah Bastawala <sarrah>
Thu 02 Mar 2023 04:37:18 PM UTC, comment #18: 

All other .oct libraries in Octave are defined in one single .cc file (+ headers). Would it make sense to merge your changes into `__ode15__.cc`?
Additionally, the .oct file libraries are not meant to be linked to by other projects. They are rather `dlopen`ed by Octave on runtime. Do we really need a header with the declarations in that case?

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 04:28:49 PM UTC, comment #17: 


> If we were also to change the Naming Style from CamelCase,I believe they could be moved to `namespace octave` as our names wouldnt clash with the defualt implementations of those functions in Sundials as well.


IIUC, you could use the exact same names as the one in SUNDIALS if you were to move those functions to the octave namespace. The one from SUNDIALS could be called as `::example_function`. And the one from Octave could be called as `octave::example_function`. Those would be different identifiers.

But I'll leave that up to you to decide.

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 04:27:19 PM UTC, comment #16: 


comment #13:

> @sarrah: Which branch/repository are you working on? I don't see any recent changes in your GitHub clone after last September.


Since this project was mostly dormant since then, there have been no commits. I am testing the changes today on my local system itself and haven't pushed to remote yet.

The current branch was https://github.com/sarrah-basta/octave/tree/gsoc-submission-final itself as pointed out by Carlo

Sarrah Bastawala <sarrah>
Thu 02 Mar 2023 04:25:16 PM UTC, comment #15: 


comment #11:

> The current naming convention is a bit confusing. With some functions starting with `OCT` or `_OCT` and some ending with `_Octave`.
> If those function have 1-to-1 correspondents in SUNDIALS, you could use that name with their conventions imho. But, I'd vote for moving them to the `namespace octave`. Use the macros `OCTAVE_BEGIN_NAMESPACE(octave)` and `OCTAVE_END_NAMESPACE(octave)` for that.


The current conventions were just based on how existing implementations in SUNDIALS using other libraries differentiated their wrappers from the default implemented functions, such as this implementation of a vector using OpenMP https://sundials.readthedocs.io/en/v6.2.0/nvectors/NVector_links.html#the-nvector-openmp-module . And for generic type names in SUNDIALS, Carlo and I had just decided to replace the `SUN` with `OCT`. Hence the confusion ...

If we were also to change the Naming Style from CamelCase,I believe they could be moved to `namespace octave` as our names wouldnt clash with the defualt implementations of those functions in Sundials as well.

Sarrah Bastawala <sarrah>
Thu 02 Mar 2023 04:23:10 PM UTC, comment #14: 


> As the current code is being written and used within Octave itself, and the functions are used in pointers, I think you are correct in saying the EXPORT macro should not be used. I have removed it and changes should reflect in the next commit.


Well, if we ever wanted to build with MSVC on Windows, we'd need to define our own `OCT_SUNDIALS_API` macro with those `dllimport` and `dllexport` attributes. But I don't know of anyone is trying to do that. And we don't need to do that for Linux or MinGW (if we don't mind that all symbols are exported - as we currently do for all .oct libraries).

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 04:10:31 PM UTC, comment #13: 

@sarrah: Which branch/repository are you working on? I don't see any recent changes in your GitHub clone after last September.

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 04:06:18 PM UTC, comment #12: 


comment #8:

> Afaict, SUNDIALS_EXPORT is defined like this in the headers:

 
Thank you for looking into that. As I was working on a Linux OS, to me it is defined as follows in the headers :


#ifdef SUNDIALS_STATIC_DEFINE
#  define SUNDIALS_EXPORT
#  define SUNDIALS_NO_EXPORT
#else
#  ifndef SUNDIALS_EXPORT
#    ifdef sundials_generic_EXPORTS
        /* We are building this library */
#      define SUNDIALS_EXPORT __attribute__((visibility("default")))
#    else
        /* We are using this library */
#      define SUNDIALS_EXPORT __attribute__((visibility("default")))
#    endif
#  endif

#  ifndef SUNDIALS_NO_EXPORT
#    define SUNDIALS_NO_EXPORT __attribute__((visibility("hidden")))
#  endif
#endif


This too dealt with permissions to expose code outside of a shared library.

> Using it like this for functions exported from a library in Octave is wrong.
> Remove it everywhere you used it.


As the current code is being written and used within Octave itself, and the functions are used in pointers, I think you are correct in saying the EXPORT macro should not be used. I have removed it and changes should reflect in the next commit.

Sarrah Bastawala <sarrah>
Thu 02 Mar 2023 04:01:51 PM UTC, comment #11: 

The current naming convention is a bit confusing. With some functions starting with `OCT` or `_OCT` and some ending with `_Octave`.
If those function have 1-to-1 correspondents in SUNDIALS, you could use that name with their conventions imho. But, I'd vote for moving them to the `namespace octave`. Use the macros `OCTAVE_BEGIN_NAMESPACE(octave)` and `OCTAVE_END_NAMESPACE(octave)` for that.

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 03:56:50 PM UTC, comment #10: 


comment #7:

> > Is that necessary?
> From within Octave this may not be necessary anymore if ida only
> accesses functions in the context via pointers ...

 
This is actually a good point I had forgotten. It does actually only calls those functions via a pointer to a struct in one of their generic types, and hence from within octave, and I now tested that it is no longer necessary to compile the code with C linkage. Thank you for pointing it out.

In this case, should I then consider moving those functions to the octave namespace, and changing their naming style, to be completely compatible with octave itself? 

Sarrah Bastawala <sarrah>
Thu 02 Mar 2023 03:50:36 PM UTC, comment #9: 

Once you removed the `extern "C"`, also change over all uses of `NULL` to `nullptr`.

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 03:27:16 PM UTC, comment #8: 

Afaict, SUNDIALS_EXPORT is defined like this in the headers:

#ifdef SUNDIALS_STATIC_DEFINE
#  define SUNDIALS_EXPORT
#  define SUNDIALS_NO_EXPORT
#else
#  ifndef SUNDIALS_EXPORT
#    ifdef sundials_generic_EXPORTS
        /* We are building this library */
#      define SUNDIALS_EXPORT __declspec(dllexport)
#    else
        /* We are using this library */
#      define SUNDIALS_EXPORT __declspec(dllimport)
#    endif
#  endif

#  ifndef SUNDIALS_NO_EXPORT
#    define SUNDIALS_NO_EXPORT
#  endif
#endif


That is used to correctly dllimport functions from the SUNDIALS library (on Windows). Using it like this for functions exported from a library in Octave is wrong.
Remove it everywhere you used it.

Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 02:37:27 PM UTC, comment #7: 


> Is that necessary?


Good question ...

It was definitely necessary during development because @sarrah initially tried using the Octave SparseMatrix class in an independent ida based application written in C.

From within Octave this may not be necessary anymore if ida only
accesses functions in the context via pointers ...


> Could you please point out where it is defined in the sources of SUNDIALS?


It's in the config file that is generated by CMake and installed
as include/sundials/sundials_config.h IIUC it is only needed on windows.

> Or alternatively (and maybe preferably), rebase on a current head of the default branch.


Yes, that would be great

Carlo de Falco <cdf>
Group Member
Thu 02 Mar 2023 11:34:06 AM UTC, comment #6: 


> As @cdf was kind to point out, SUNDIALS_EXPORT is a macro in the C API of SUNDIALS and not defined by me. It is used to expose the replacement functions created by me (which replace what would ideally be SUNDIALS functions) to the rest of the SUNDIALS API.


Could you please point out where it is defined in the sources of SUNDIALS?

> are hence exposed by `#extern C`.


Is that necessary?


Markus Mützel <mmuetzel>
Group administrator
Thu 02 Mar 2023 10:12:30 AM UTC, comment #5: 

comment #3:

> @mmuetzel thanks for looking into this.
>
>  - I don't have time right now to respond to all your comments, I hope maybe @sarrah can?

 
Yes, thank you @mmuetzel and @cdf for your comments and looking into this.


For @mmuuetzel 's comments :
comment #2:

> * I can't see where SUNDIALS_EXPORT is defined. What is it supposed to do?


As @cdf was kind to point out, SUNDIALS_EXPORT is a macro in the C API of SUNDIALS and not defined by me. It is used to expose the replacement functions created by me (which replace what would ideally be SUNDIALS functions) to the rest of the SUNDIALS API. 

> * CamelCase in function names is discouraged in Octave's sources. Use all lower case with underscores as separators (unless there is a good reason to deviate from that convention here).
> * Many functions have names that contain "OCT" or "_Octave". Would it make sense to move them to the `octave` namespace instead?


Regarding the above two points, as @cdf pointed out, most of these are with regards to using the SUNDIALS C API explained above, as those functions that are written in C++, still have to be called by the  C API, and are hence exposed by `#extern C`. Hence I too do not think using such C++ functionality will be possible.

> * Remove the include guard `__CONFIG_H_INCLUDE_GUARD` from `__ode15__.cc`. `.cc` files aren't supposed to be included in other files normally. Is this different for this file?
> * Revert the white space changes in your patch for the pre-processor macros. The hash `#` is supposed to be in column 1. Spaces for indentation follow after the `#`. Use two spaces for indentation.
> * Avoid using old-style cast in NV_CONTENT_C. Use `reinterpret_cast` instead. Or does that not work for some reason here?
> * Use a space between function name and following opening parenthesis.

 
With regards to these points, this patch is a slightly older version, and I did make some changes to the code after this especially to accommodate the Coding Style. These are stored on the git repository mentioned by @cdf and the changes are much clearly discussed by me at : https://octave.discourse.group/t/advice-for-final-patch-submission-from-github-mirror/3232/3?u=sarrah-basta

If you could have a look at these it would be great. I will myself try ensuring that I have followed all the style guide comments you posted about and try rebasing my code on a current head on the default branch of the git mirror and reply soon.

Sarrah Bastawala <sarrah>
Wed 01 Mar 2023 05:30:41 PM UTC, comment #4: 


> from quickly reading your comments I guess you are not considering that @sarrah was using the C API of SUNDIALS so I'm not sure that using some C++ feature (such as e.g. namespaces) is feasible ...


Maybe I'm missing the point. But those functions are implemented in C++. C++ can call C functions. And iiuc function pointer to C++ functions can be passed to C functions. Why would interfacing to C prevent this code from using namespaces?

> the first things to do would be to merge default into that branch


Or alternatively (and maybe preferably), rebase on a current head of the default branch.

Markus Mützel <mmuetzel>
Group administrator
Wed 01 Mar 2023 03:41:22 PM UTC, comment #3: 

@mmuetzel thanks for looking into this.

 - I don't have time right now to respond to all your comments, I hope maybe @sarrah can?

 - from quickly reading your comments I guess you are not considering that @sarrah was using the C API of SUNDIALS so I'm not sure that using some C++ feature (such as e.g. namespaces)
is feasible ...

 - Also some naming conventions are consistent with the SUNDIALS
coding style rather than Octave's


 - The GIT repo used by @sarrah for her project is still available here : https://github.com/sarrah-basta/octave
I think this is the branch used for the final submission : https://github.com/sarrah-basta/octave/tree/gsoc-submission-final maybe one of the first things to do would be to merge default into that branch and see wether it still works?


Carlo de Falco <cdf>
Group Member
Wed 01 Mar 2023 12:41:48 PM UTC, comment #2: 

I haven't tested the patch. Just some initial remarks:

  • I can't see where SUNDIALS_EXPORT is defined. What is it supposed to do?
  • CamelCase in function names is discouraged in Octave's sources. Use all lower case with underscores as separators (unless there is a good reason to deviate from that convention here).
  • Many functions have names that contain "OCT" or "_Octave". Would it make sense to move them to the `octave` namespace instead?
  • Remove the include guard `__CONFIG_H_INCLUDE_GUARD` from `__ode15__.cc`. `.cc` files aren't supposed to be included in other files normally. Is this different for this file?
  • Revert the white space changes in your patch for the pre-processor macros. The hash `#` is supposed to be in column 1. Spaces for indentation follow after the `#`. Use two spaces for indentation.
  • Avoid using old-style cast in NV_CONTENT_C. Use `reinterpret_cast` instead. Or does that not work for some reason here?
  • Use a space between function name and following opening parenthesis.


Markus Mützel <mmuetzel>
Group administrator
Tue 13 Sep 2022 09:24:26 AM UTC, comment #1: 

Here are a few additional relevant links associated with this project :

Discussion with the Community : https://octave.discourse.group/t/gsoc-2022-project-ode15-i-s/1343?u=sarrah-basta

Progress documentation throughout the Project : https://sarrah-basta.github.io/blogs/

Final Project Report for GSoC Submission : https://sarrah-basta.github.io/blogs/2022/09/11/GSoC_'22_GNU_Octave_Project_Report.html

Also attaching an updated version (v2) of the patch for the project.

(file #53687)

Sarrah Bastawala <sarrah>
Mon 12 Sep 2022 04:04:12 PM UTC, original submission:  

Hi,
The attached patch implements the added features of making the KLU dependency optional for ode15{i,s} DAE solvers into Octave core.

This is the result of my (Sarrah Bastawala 's) GSOC 2022 project mentored by Carlo de Falco that has been kept up to date in a separate github repository until now.

The repository is here:
https://github.com/sarrah-basta/octave.git

The changelog message to best describe the code is as follows :


add support for sparse computations in ode15{i,s} using Octave classes, making the dependency on SUNDIALS interface to KLU optional ( GSoC '22)

* libinterp/dldfcn/oct-sundials.h : add declarations of custom implementations to SUNDIALS NVECTOR, SUNMATRIX and SUNLINEARSOLVER APIs using Octave's classes.
* libinterp/dldfcn/oct-sundials.cc: add definitions of exported functions declared in corresponding header file
* libinterp/dldfcn/__ode15__.cc : add motifications and optimizations to make ode15{i,s} solvers default to using Octave's custom implementations and solver in IDA for sparse computations if KLU is not found.binterp
* libinterp/dldfcn/config-module.awk : add build dependencies for __ode15__ oct-file.
* configure.ac : modify warnings to let user know ode15{i,s} support sparse computations even if KLU is not found.
* scripts/ode/ode15i.m : modify tests using sparse Jacobians to run even without KLU
* scripts/ode/ode15s.m : modify tests using sparse Jacobians to run even without KLU
---
 configure.ac                       |    4 +-
 libinterp/dldfcn/__ode15__.cc      |  295 ++++++--
 libinterp/dldfcn/config-module.awk |   10 +-
 libinterp/dldfcn/oct-sundials.cc   | 1134 ++++++++++++++++++++++++++++
 libinterp/dldfcn/oct-sundials.h    |  244 ++++++
 scripts/ode/ode15i.m               |    4 +-
 scripts/ode/ode15s.m               |   10 +-
 7 files changed, 1618 insertions(+), 83 deletions(-)


Sarrah Bastawala <sarrah>

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by sarrah (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 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-10-13 siko1056 Summaryadd support for sparse computations in ode15{i,s} using Octave classes , making dependency on KLU optional. GSoC 2022: add support for sparse computations in ode15{i,s} using Octave classes , making dependency on KLU optional.
    2022-09-13 sarrah Attached File- Added final_patch_gsoc_ode15i_2022_v2.txt, #53687
    2022-09-12 sarrah Attached File- Added final_patch_gsoc_ode15i_2022.txt, #53681

    Back to the top

    Powered by Savane 3.12