don't merge yet - reduce number of HAVE_NARRAY_H occurences

[minad]

Following https://github.com/SciRuby/rb-gsl/issues/14 I tried to cut down the number of narray occurences to see how many there really are. grep -r HAVE_NARRAY_H | wc went down from 216 to 130. That's still a lot. But I think it would be better to try at first to carefully separate gsl and narray code to maybe achieve nmatrix/narray compatibility at some point, while still keeping it working in its current form.

I am not trying to create another fork here ;) These are trivial code changes, basically just removing some macros.

@v0dro @blackwinter what do you think?

[minad]

Code review

I went a bit through the code - this is what I found:

Code duplication

There is an incredible amount of code duplication. Parameter conversion/unpacking/...

...
    if (NA_IsNArray(xx)) {                                                                                                                                                                                                                   
      struct NARRAY *na;                                                                                                                                                                                                                     
      double *ptr1, *ptr2;                                                                                                                                                                                                                   
      xx = na_change_type(xx, NA_DFLOAT);                                                                                                                                                                                                    
      GetNArray(xx, na);                                                                                                                                                                                                                     
      ptr1 = (double *) na->ptr;                                                                                                                                                                                                             
      n = na->total;                                                                                                                                                                                                                         
      ary = na_make_object(NA_DFLOAT, na->rank, na->shape, CLASS_OF(xx));                                                                                                                                                                    
      ptr2 = NA_PTR_TYPE(ary, double*);                                                                                                                                                                                                      
      for (i = 0; i < n; i++) ptr2[i] = (*f)(ptr1[i]);                                                                                                                                                                                       
      return ary;                                                                                                                                                                                                                            
    }                                                                                                                                                             
...                                                                           
    if (NA_IsNArray(xx)) {                                                                                                                                                                                                                   
      struct NARRAY *na;                                                                                                                                                                                                                     
      double *ptr1, *ptr2;                                                                                                                                                                                                                   
      xx = na_change_type(xx, NA_DFLOAT);                                                                                                                                                                                                    
      GetNArray(xx, na);                                                                                                                                                                                                                     
      ptr1 = (double *) na->ptr;                                                                                                                                                                                                             
      n = na->total;                                                                                                                                                                                                                         
      ary = na_make_object(NA_DFLOAT, na->rank, na->shape, CLASS_OF(xx));                                                                                                                                                                    
      ptr2 = NA_PTR_TYPE(ary, double*);                                                                                                                                                                                                      
      for (i = 0; i < n; i++) ptr2[i] = (*f)(ptr1[i], a);                                                                                                                                                                                    
      return ary;                                                                                                                                                                                                                            
    }                                                                                                                                                             
...                                                                           
    if (NA_IsNArray(xx)) {                                                                                                                                                                                                                   
      struct NARRAY *na;                                                                                                                                                                                                                     
      double *ptr1, *ptr2;                                                                                                                                                                                                                   
      xx = na_change_type(xx, NA_DFLOAT);                                                                                                                                                                                                    
      GetNArray(xx, na);                                                                                                                                                                                                                     
      ptr1 = (double *) na->ptr;                                                                                                                                                                                                             
      n = na->total;                                                                                                                                                                                                                         
      ary = na_make_object(NA_DFLOAT, na->rank, na->shape, CLASS_OF(xx));                                                                                                                                                                    
      ptr2 = NA_PTR_TYPE(ary, double*);                                                                                                                                                                                                      
      for (i = 0; i < n; i++) ptr2[i] = (*f)(ptr1[i], a, b);                                                                                                                                                                                 
      return ary;                                                                                                                                                                                                                            
    }
...      

However it seems that the code duplication is sometimes really like 100% exact. This should be very easy and fast to cleanup. This code be done using some kind of intelligent tooling to detect similar code snippets.

I've seen such repetitive code very often, especially in language bindings. I know, macros are sometimes frowned upon but what if they reduce the amount of code by 66%?

Variable scoping

The functions are sometimes quite large and all of them specify the variables at the beginning. I think it is better to keep variables local. This is also what I did in this patch since this makes it much easier to refactor.

But this is a matter of coding style.

GSL version checks for very old GSL versions

On debian stable we have GSL 1.15. Could we at least rely on that?

Missing tests for NArray

There is also another problem - not enough tests! There are nearly no tests which check if the functions accept NArray etc. Makes it hard to do refactoring. This would need quite some effort.

If tests for nmatrix are written, we can also write them for narray at the same time.

[v0dro]

This looks good to me. But what will happen once we try to create a fork that can with both nmatrix and narray? Wont the macros be helpful in that case?

Maybe we can have some global macros that factor out the nmatrix/narray functionality completely and call the relevant code from the nmatrix or narray C APIs, such that the specific code is abstracted from a programmer and he only sees the macros.

So for instance to access the elements in an NMatrix or NArray object:

#ifdef HAVE_NARRAY_H
  ACCESS_ELEMENTS(narray, type) { (type)narray->elements; } // return an array of elements
#endif

#ifdef HAVE_NMATRIX_H
  ACCESS_ELEMENTS(nmatrix,type) { (type)DENSE_ELEMENTS(nmatrix) } // return array of elements
#endif

// and in the calling code...

matrix = // init nmatrix or narray
// ... some logic
if (matrix is nmatrix or narray)
  arr = ACCESS_ELEMENTS(matrix, double)

@minad maybe, instead of simply removing the macros and placing the relevant code in the right place, if you could begin by creating these abstractions and make this work only for NArray initially, it would serve as a proof of concept and then we can easily introduce NMatrix functionality.

[minad]

@v0dro I am not removing anything of relevance. I am just restructuring the code slightly here to reduce the number of occurences of narray. Later, this will make it easier to port the relevant pieces to nmatrix.

[blackwinter]

Hi Daniel,

I really appreciate your effort. Unfortunately, I'm not sure how much resources I can put into this going further. As you say, the code quality is not the greatest and it would need a lot of work to get it right. I only created this fork to add support for newer Ruby and GSL versions; with the attention it's getting lately, I feel a bit overwhelmed.

Would you be willing to take responsibility for merging the changes yourself? Meaning, I would give you commit access, but look over commits coming in and be available for cutting releases (for the time being); I would just refrain from making code-related decisions.

Cheers, Jens

[blackwinter]

On debian stable we have GSL 1.15. Could we at least rely on that?

I would support the removal of outdated versions. 1.15 and higher seems fine.

(EDIT: The same holds for old Ruby versions. 2 and higher should be sufficient.)

[minad]

Ok, if you give me access I will merge this and further improvements concerning compatibility. Maybe I can coordinate with @v0dro how to continue the project.

Do you want to review the changes?

[blackwinter]

Great, I've added you to the project! Let me know if you need anything.

I'll certainly try to review your commits and comment on them if necessary, but we don't have to establish formal code review if that's what you meant.

[v0dro]

Sure we'll keep in touch.