-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
An R package that wraps your code #10
Comments
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.
#5 (comment)
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 ***@***.***> 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
<#10>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKQRXIUBY4OTO55CXFJWBTRIDV6FANCNFSM4LOU2VSA>
.
|
Of course
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
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. |
On Wed, 18 Mar 2020 at 18:52, Jean-Romain ***@***.***> 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
<#10 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKQRXJMBRMRS6Z33X5M2L3RIEC7JANCNFSM4LOU2VSA>
.
|
Ok for speed it is my fault. It a mistake I added I my R configuration and |
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
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.
Thank you
The text was updated successfully, but these errors were encountered: