sadaszewski / concaveman-cpp

C++ port of mapbox's JS concaveman, with a Python wrapper
BSD 2-Clause "Simplified" License
151 stars 40 forks source link

An R package that wraps your code #10

Open Jean-Romain opened 4 years ago

Jean-Romain commented 4 years ago

Hi,

I wrapped your C++ code into an package for the R language. See RcppConcaveman. Then I tried it and I compared it to the R package concaveman that is a wrapper to the original JavaScript code from mapbox using V8.

I found that the output of your implementation is different than the one from the original version. See image below. The read is the original one, the blue is yours

Rplot

Also I benchmarked it and I found that it was slower than the original JS code. See RcppConcaveman.

I'd like to have your opinion of these two points.

  1. Do you have an idea where the two implementations diverge?
  2. Do you have an idea where your implementation can be speed-up?

Thank you

sadaszewski commented 4 years ago

Hi Jean-Romain,

Thank you for taking interest in my implementation of concaveman. We should determine what exactly is going on by implementing unit tests for all the routines that are present in concaveman-cpp and compare their outputs to the JS homologues. Small deviations can accumulate over the course of a complex algorithm like this and in the end give seemingly very different results.

As to the speed-up I had different experience myself, as well as reported by other users, e.g. https://github.com/sadaszewski/concaveman-cpp/issues/5#issuecomment-537427384

I would prioritize the unit tests before looking into performance. If you were willing, we could work on this together, albeit pretty slowly on my side because I am seriously overloaded. What do you think?

Best regards,

-- Stanislaw

On Wed, 18 Mar 2020 at 17:01, Jean-Romain notifications@github.com wrote:

Hi,

I wrapped your C++ code into an package for the R language. See RcppConcaveman https://github.com/Jean-Romain/RcppConcaveman. Then I tried it and I compared it to the R package concaveman https://github.com/joelgombin/concaveman that is a wrapper to the original JavaScript code from mapbox using V8 https://cran.r-project.org/web/packages/V8/index.html.

I found that the output of your implementation is different than the one from the original version. See image below. The read is the original one, the blue is yours

[image: Rplot] https://user-images.githubusercontent.com/3872279/76980276-62b9ef80-690f-11ea-87a3-005c81a36049.jpeg

Also I benchmarked it and I found that it was slower than the original JS code. See RcppConcaveman https://github.com/Jean-Romain/RcppConcaveman#benchmarks.

I'd like to have your opinion of these two points.

  1. Do you have an idea where the two implementations diverge?
  2. Do you have an idea where your implementation can be speed-up?

Thank you

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sadaszewski/concaveman-cpp/issues/10, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQRXIUBY4OTO55CXFJWBTRIDV6FANCNFSM4LOU2VSA .

Jean-Romain commented 4 years ago

I would prioritize the unit tests before looking into performance.

Of course

we could work on this together

My knowledge about JS is limited to few scripts copy/pasted for websites in the early 2000's but yes why not. I started to clean your code. I mean, I put the rtree in a separate file and I re-indented the code in a way I can read it more easily. So I can now go more in depth in the code.

As to the speed-up I had different experience myself, as well as reported by other users, e.g.

So somebody already wrapped your code in a R package. The code is strictly identical to mine but yet I can reproduce the benchmark and the version proposed by this user is 5x faster. I will investigate. It's weird.

sadaszewski commented 4 years ago

On Wed, 18 Mar 2020 at 18:52, Jean-Romain notifications@github.com wrote:

I would prioritize the unit tests before looking into performance.

Of course

we could work on this together

My knowledge about JS is limited to few scripts copy/pasted for websites in the early 2000's but yes why not. I started to clean your code. I mean, I put the rtree in a separate file and I re-indented the code in a way I can read it more easily. So I can now go more in depth in the code.

Perfect. Looking at rtree would be a great place to start. Unit tests for rbush (the library used by JS concaveman) are available here:

https://github.com/mourner/rbush/blob/master/test/test.js

It would be a great first step to implement the same in C++. Please let me know if you think this is feasible.

Best regards,

-- Stanislaw

As to the speed-up I had different experience myself, as well as reported by other users, e.g.

So somebody already wrapped your code in a R package. The code is strictly identical to mine but yet I can reproduce the benchmark and the version proposed by this user is 5x faster. I will investigate. It's weird.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sadaszewski/concaveman-cpp/issues/10#issuecomment-600775445, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQRXJMBRMRS6Z33X5M2L3RIEC7JANCNFSM4LOU2VSA .

Jean-Romain commented 4 years ago

Ok for speed it is my fault. It a mistake I added I my R configuration and g++ was not longer compiling with -02. One question solved.

Jean-Romain commented 4 years ago

For your information. Your version is 50 times faster when there are very few points (probably because of the overhead introduced by calling JavaScript in R via a V8 wrapper) and tend to be 2 times faster with a lot of points.

timing