Merge with rb-gsl

[agarie]

Hi folks,

I'm using a Ruby gem that depends on rb-gsl (glove) and decided to see if it would work with gsl-nmatrix as well. Turns out I couldn't even install it correctly:

λ gem install gsl-nmatrix                                                                                                                                                                                                                                                 Fetching: gsl-nmatrix-1.15.3.2.gem (100%)
Building native extensions.  This could take a while...
ERROR:  Error installing gsl-nmatrix:
    ERROR: Failed to build gem native extension.

    /Users/agarie/.rubies/ruby-2.2.0/bin/ruby -r ./siteconf20150424-7363-1opqasp.rb extconf.rb
checking gsl version... 1.16
checking gsl cflags... -I/usr/local/Cellar/gsl/1.16/include

[...]

In file included from gsl_nmatrix.c:10:
../include/rb_gsl_with_nmatrix.h:3:10: fatal error: 'nmatrix.h' file not found
#include "nmatrix.h"
         ^
1 error generated.
make: *** [gsl_nmatrix.o] Error 1

make failed, exit code 2

I don't have time to find why it isn't working right now (IIRC @v0dro had the same problem with travis-ci), but that made me think: was there a very strong reason to not merge this fork with the current rb-gsl gem?

Apparently, the current maintainer is @blackwinter: the rubygems page points to blackwater/rb-gsl.

[ktns]

I think the issue you encountered is related to #13.
I'm also trying to blackwinter/rb-gsl into gsl-nmatrix, but it takes a little more time.

[blackwinter]

I'd love to join forces somehow. Unfortunately, both our implementations deviated pretty extensively from the original code base which won't make it any easier to merge the two.

Also, I have a feeling that gsl-nmatrix cannot simply take rb-gsl's place (which would have been my favoured solution as I'm not really rooted in GSL land). I'm sure it's great, but the installation requirements for nmatrix are quite a bit more involved than narray's.

This means we might want to try and find a way to either combine both narray and nmatrix based implementations (optimal, but hard) or collaborate and maintain compatibility (including bug fixes) across forks (easier, but also inferior).

What do you think?

[v0dro]

NMatrix is on its way to be very easily installable. We're working on removing all the dependencies and making it an independent, dependency-free gem.

Thus I think gsl-nmatrix can potentially replace rb-gsl once that is done (I think it will be a GSOC project).

[blackwinter]

That sounds awesome! I'll be sure to keep an eye on it (ref).

[agarie]

Yep, there's a lot of work going into making NMatrix easy to install. This fork was created to allow both NArray and NMatrix to coexist afaik, so it should be possible (but not necessarily easy) to implement the same changes in the current rb-gsl repository.

@blackwinter the GSoC students are going to be announced today, so there's going to be some posts on our mailing list regarding that project soon. :)

[blackwinter]

Hi Sameer,

first of all, I'm really glad that you've taken on the task to make the current rb-gsl gem work with NMatrix instead of NArray. I put quite a bit of work into my fork, but ultimately feel that it should be in the hands of more active users. So I'd be more than happy to see it under the @SciRuby umbrella.

Regarding your recent work on gsl-nmatrix I have a few comments. I'll keep them in this issue for now, but we can move the discussion elsewhere if you prefer.

1. It's a bit unfortunate that you didn't keep the commit metadata or at least the commit structure of @MohawkJohn's changes (or my most recent ones, for that matter). I know it's hard to merge two actively developed projects like this, especially considering that it's not even the main objective of your GSoC project.
2. Your code in extconf.rb fails with a NoMethodError when the nmatrix gem could not be found. Besides, the work gsl_gem_config does happens for a reason (actually two). If that doesn't work for nmatrix, it should be amended instead of abandoned.
3. In applying the switch, you commented out a few passages. That should probably be cleaned up or even fixed in cases the sole reason was that it broke something. Also, in renaming ext/gsl/include/rb_gsl_with_narray.{c -> old} and ext/gsl/include/rb_gsl_with_narray.{h -> old} that process was not recorded with Git (and why keep the old files in the first place?).
4. There are some references to NArray left in the code (about 40 files).
5. When narray is installed, require 'nmatrix' picks up the nmatrix.rb from narray. This is definitely a mistake by narray, but should possibly be dealt with.
6. Last but not least, there were non-switch related and debug changes mixed in with everything else. Maybe a clearer structure would simplify the discussion and improve future maintenance.

I don't want to come across as too nitpicky, but seeing as this should also be an opportunity to learn, I hope you take my comments in good faith.

As for the path forward, I'm excited that your fork might become the new SciRuby/rb-gsl, as John already indicated, and hence the canonical implementation. As soon as nmatrix's installation requirements have been softened (see @wlevine's GSoC project), I'd be willing to give up the rb-gsl gem name and point my repository here. Peripheral changes (upgrade path for current users, maybe GSL-independent versioning) notwithstanding.

Would that be agreeable to everyone?

Cheers,
Jens

[v0dro]

I really appreciated your comments. They're very useful for me to learn more.

The thing is, my GSOC project is primarily concerned with statsample and daru, and statsample heavily depends on rb-gsl (and narray) and I cannot properly integrate daru and statsample if that is the case.

The reason why the gem seems to be in a shabby state is because I just wanted to make sure that it works with nmatrix and statsample, so that I can begin my work ASAP. I have committed to @agarie that I will be porting the narray dependency to nmatrix completely in the next few weeks, whenever I get a breather from my main GSOC work.

As such you can expect gsl-nmatrix upto your standards in 3 or 4 months max.

I've addressed all your concerns in the points that follow:

1. I will do a proper merge (i.e. by keeping the histories of both @MohawkJohn's changes in SciRuby/rb-gsl and yours in blackwinter/rb-gsl) once gsl-nmatrix are stable and I get a breather during GSOC. This will be done using the proper versioning tools, unlike now where I've manually copy pasted John's code into yours.
2. Again this is a result of just getting things to work. Will update.
3. Will remove narray files completely later, as I said.
4. I honestly dont know how to get rid of this one. Perhaps @MohawkJohn or @agarie can help?

Cheers!

[translunar]

We've been struggling with (5) for a while. No one seems to know what to do about it. If you have an idea, we'd love to implement it. I suspect, though, that it requires a change in NArray, and the author is not eager to do that since his gem predates ours.

[blackwinter]

@v0dro: I totally understand that this is only tangential to your project. What I don't understand is, why can't you work with gsl-nmatrix in its current form (or even without GSL, ref: SciRuby/statsample#18, SciRuby/statsample#28, SciRuby/statsample#31)? Anyway, maybe we should just put it on hold for now and continue when you're finished with GSoC - if you're still interested by then ;) Please don't let my comments pressure you in any way.

@MohawkJohn: I would say that narray is in violation of the implicit contract, but so is rb-gsl (with ool.rb and maybe even rbgsl.rb). Cleaning up those mishaps after the fact isn't easy. (For the record: SciRuby/nmatrix#6, SciRuby/nmatrix#172 and masa16/narray#39 deal with this issue. narray-nmatrix has been proposed as a workaround, but I fail to see how that should work; it still loads narray-nmatrix's nmatrix.rb instead of nmatrix's.)

A few ideas spring to mind, though:

1. Wait for the Next NArray, which doesn't seem to have that problem anymore.
2. In user code, activate the nmatrix gem first: gem 'nmatrix'; require 'nmatrix'.
3. In nmatrix, provide a different entry point so that the nmatrix gem will be activated automatically: require 'nmatrix/because_i_say_so' (you get the gist ;). This would mean moving the current matrix.rb to that new file and making matrix.rb a stub that just requires it. (EDIT: User code would still have to be adjusted to use the new entry point, of course.)

[v0dro]

gsl-nmatrix in its current form was outdated, and I was facing big problems working with the setup.rb file and making the tests run, ensuring consistent builds and making changes etc. To top it all John's code depended on a very old version of nmatrix and the nmatrix C API has changed quite a bit since then.

Once I saw that you had sufficiently updated rb-gsl to be a modern and robust gem, I was eager to port this stuff there. Plus my official coding period begins next week so this was a good opportunity to get familiar with the code base.

[blackwinter]

I see, thanks for the background. Good luck with your project!

[translunar]

@blackwinter (3) is an interesting idea.

Could we do require "nmatrix/seriously" and then have that file just do require "./nmatrix.rb" or something like that, so it isn't ambiguous?

[blackwinter]

Could we do require "nmatrix/seriously" and then have that file just do require "./nmatrix.rb" or something like that, so it isn't ambiguous?

That would be equivalent to require 'nmatrix/nmatrix', so the top-level nmatrix.rb would not be loaded at all. (EDIT: Not sure if you meant that, but require_relative '../nmatrix' would work.)

But nmatrix.rb only contains requires, among which is nmatrix/nmatrix.rb. Why not simply move those requires to nmatrix/nmatrix.rb and only leave require 'nmatrix/nmatrix'? This way both require 'nmatrix' and require 'nmatrix/nmatrix' work out the same in user code, with the latter also working in the presence of narray's nmatrix.rb.

I can prepare a pull request if you like, although it would probably have to wait till Monday.

[translunar]

That'd be great.
On Fri, May 8, 2015 at 4:51 PM Jens Wille notifications@github.com wrote:

Could we do require "nmatrix/seriously" and then have that file just do
require "./nmatrix.rb" or something like that, so it isn't ambiguous?

That would be equivalent to require 'nmatrix/nmatrix', so the top-level
nmatrix.rb would not be loaded at all.

But nmatrix.rb only contains requires, among which is nmatrix/nmatrix.rb.
Why not simply move those requires to nmatrix/nmatrix.rb and only leave require
'nmatrix/nmatrix'? This way both require 'nmatrix' and require
'nmatrix/nmatrix' work out the same in user code, with the latter also
working in the presence of narray's nmatrix.rb.

I can prepare a pull request if you like, although it would probably have
to wait till Monday.

—
Reply to this email directly or view it on GitHub
#14 (comment).

[minad]

@blackwinter @v0dro @MohawkJohn It seems there are now three different diverging repositories (gems):

• https://github.com/SciRuby/rb-gsl (gsl-nmatrix)
• https://github.com/blackwinter/rb-gsl (rb-gsl)
• https://github.com/v0dro/rb-gsl (port of blackwinter's rb-gsl to nmatrix)

Would it be possible to make blackwinter's rb-gsl compatible with both narray/nmatrix and compile support depending on what is available? I think that would be the best to keep compatibility with both libraries and to avoid such diverging repositories in the future.

[v0dro]

Hmmm...compatibility with both narray and nmatrix seems interesting, but it would lead to a lot of messy code in rb-gsl.

Also, what if nmatrix and narray diverge in functionality is some aspects?

IMO changing @blackwinter's rb-gsl to support nmatrix would be the way to go.

[minad]

Hi Sameer,

I think the only narray/nmatrix specific part is the conversion between those and gsl matrices. So I think this can be done in a clean way using some macro functions.

My main point is that the current situation is unsatisfactory. As I said there are the thee forks of yours around and not to forget the original gsl gem. All are incompatible and diverged quite a lot. Lost effort?

Under the assumption that both narray/nmatrix will continue to exist we need one single implementation which can be released and maintained by all the current contributors to rb-gsl/gsl-nmatrix/gsl.

I think it is much less effort to maintain the binding by itself + a few narray/nmatrix specifics instead of maintaining 2 different gsl bindings.

However I haven't worked on the codebase. Those who have can judge better. Maybe you could try to cleanup your fork in such a way that you avoid doing the trivial things like readme whitespace etc so to merge cleanly.
Then we can also see how much narray/nmatrix specific code there really is.

Daniel

Am 12. Juni 2015 09:06:44 MESZ, schrieb Sameer Deshmukh notifications@github.com:

Hmmm...compatibility with both narray and nmatrix seems interesting,
but it would lead to a lot of messy code in rb-gsl.

Also, what if nmatrix and narray diverge in functionality is some
aspects?

IMO changing @blackwinter's rb-gsl to support nmatrix would be the way
to go.

---

Reply to this email directly or view it on GitHub:
#14 (comment)

[agarie]

I agree with Daniel in that, if both NArray and NMatrix continue to exist,
it makes sense to enable rb-gsl to support both.

By the way, if this discussion interests you, take a look at Daniel's PR to
rb-gsl: blackwinter/rb-gsl#23.

---

Carlos Agarie
Data Scientist at Tapps Games (http://tappsgames.com)
+55 11 97320-3878 | @carlos_agarie

2015-06-12 6:28 GMT-03:00 Daniel Mendler notifications@github.com:

Hi Sameer,

I think the only narray/nmatrix specific part is the conversion between
those and gsl matrices. So I think this can be done in a clean way using
some macro functions.

My main point is that the current situation is unsatisfactory. As I said
there are the thee forks of yours around and not to forget the original gsl
gem. All are incompatible and diverged quite a lot. Lost effort?

Under the assumption that both narray/nmatrix will continue to exist we
need one single implementation which can be released and maintained by all
the current contributors to rb-gsl/gsl-nmatrix/gsl.

I think it is much less effort to maintain the binding by itself + a few
narray/nmatrix specifics instead of maintaining 2 different gsl bindings.

However I haven't worked on the codebase. Those who have can judge better.
Maybe you could try to cleanup your fork in such a way that you avoid doing
the trivial things like readme whitespace etc so to merge cleanly.
Then we can also see how much narray/nmatrix specific code there really is.

Daniel

Am 12. Juni 2015 09:06:44 MESZ, schrieb Sameer Deshmukh <
notifications@github.com>:

Hmmm...compatibility with both narray and nmatrix seems interesting,
but it would lead to a lot of messy code in rb-gsl.

Also, what if nmatrix and narray diverge in functionality is some
aspects?

IMO changing @blackwinter's rb-gsl to support nmatrix would be the way
to go.

---

Reply to this email directly or view it on GitHub:
#14 (comment)

—
Reply to this email directly or view it on GitHub
#14 (comment).

[blackwinter]

I also agree that a well-abstracted code base with support for both NArray and NMatrix would be the best solution overall. I just can't put in the necessary work right now. If others step up to do it, I'd be more than happy to assist in any way I can.