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

getItemsVector crashes R #24

Closed
pommedeterresautee opened this Issue Sep 23, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@pommedeterresautee

pommedeterresautee commented Sep 23, 2017

Calling getItemsVector make R crash. BTW getItemsVector is not in the tests :-)

library(RcppAnnoy)
set.seed(123)
f <- 40
a <- new(AnnoyEuclidean, f)
n <- 50                                
for (i in seq(n)) {
    v <- rnorm(f)
    a$addItem(i-1, v)
}
a$build(50)                   
a$save("/tmp/test.tree")
b <- new(AnnoyEuclidean, f)   
b$load("/tmp/test.tree")	
print(b$getNNsByItem(0, 40))
b$getItemsVector(0) # <- CRASH !!!
> devtools::session_info()
Session info -----------------------------------------------------------------------------------------------------------
 setting  value                       
 version  R version 3.4.1 (2017-06-30)
 system   x86_64, linux-gnu           
 ui       RStudio (1.1.350)           
 language fr_FR:en                    
 collate  fr_FR.UTF-8                 
 tz       Europe/Paris                
 date     2017-09-23                  

Packages ---------------------------------------------------------------------------------------------------------------
 package   * version date       source        
 base      * 3.4.1   2017-07-08 local         
 codetools   0.2-15  2016-10-05 CRAN (R 3.3.1)
 compiler    3.4.1   2017-07-08 local         
 datasets  * 3.4.1   2017-07-08 local         
 devtools    1.13.3  2017-08-02 CRAN (R 3.4.1)
 digest      0.6.12  2017-01-27 CRAN (R 3.4.0)
 graphics  * 3.4.1   2017-07-08 local         
 grDevices * 3.4.1   2017-07-08 local         
 memoise     1.1.0   2017-04-21 CRAN (R 3.4.0)
 methods   * 3.4.1   2017-07-08 local         
 Rcpp        0.12.12 2017-07-15 CRAN (R 3.4.1)
 RcppAnnoy * 0.0.9   2017-08-31 CRAN (R 3.4.1)
 stats     * 3.4.1   2017-07-08 local         
 tools       3.4.1   2017-07-08 local         
 utils     * 3.4.1   2017-07-08 local         
 withr       2.0.0   2017-07-28 CRAN (R 3.4.1)
 yaml        2.1.14  2016-11-12 CRAN (R 3.4.0)
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 23, 2017

Owner

Fichtre. Confirmed.

@erikbern Any idea from the top of your head? I am doing

    std::vector<double> getItemsVector(int32_t item) {
        std::vector<float> fv;
        ptr->get_item(item, &fv[0]);
        std::vector<double> dv(fv.size());
        std::copy(fv.begin(), fv.end(), dv.begin());
        return dv;
    }

which is pretty straight up from your annoymodule. But clearly fv is not set up here so that function makes little sense as written. Hmpf.

Owner

eddelbuettel commented Sep 23, 2017

Fichtre. Confirmed.

@erikbern Any idea from the top of your head? I am doing

    std::vector<double> getItemsVector(int32_t item) {
        std::vector<float> fv;
        ptr->get_item(item, &fv[0]);
        std::vector<double> dv(fv.size());
        std::copy(fv.begin(), fv.end(), dv.begin());
        return dv;
    }

which is pretty straight up from your annoymodule. But clearly fv is not set up here so that function makes little sense as written. Hmpf.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 23, 2017

Owner

Tears go away if fv is initialized with proper size, ie

diff --git a/src/annoy.cpp b/src/annoy.cpp
index 04aa5d7..db5a4cd 100644
--- a/src/annoy.cpp
+++ b/src/annoy.cpp
@@ -113,7 +113,7 @@ public:
     }

     std::vector<double> getItemsVector(int32_t item) {
-        std::vector<float> fv;
+        std::vector<float> fv(vectorsz);
         ptr->get_item(item, &fv[0]);
         std::vector<double> dv(fv.size());
         std::copy(fv.begin(), fv.end(), dv.begin());

I'll make that change. Thanks to @pommedeterresautee for the bug report!

Owner

eddelbuettel commented Sep 23, 2017

Tears go away if fv is initialized with proper size, ie

diff --git a/src/annoy.cpp b/src/annoy.cpp
index 04aa5d7..db5a4cd 100644
--- a/src/annoy.cpp
+++ b/src/annoy.cpp
@@ -113,7 +113,7 @@ public:
     }

     std::vector<double> getItemsVector(int32_t item) {
-        std::vector<float> fv;
+        std::vector<float> fv(vectorsz);
         ptr->get_item(item, &fv[0]);
         std::vector<double> dv(fv.size());
         std::copy(fv.begin(), fv.end(), dv.begin());

I'll make that change. Thanks to @pommedeterresautee for the bug report!

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 23, 2017

Owner

For completeness:

edd@brad:~/git/rcppannoy(master)$ Rscript /tmp/annoycrash.R
 [1]  0 28 17 19 49 32 38 31 47 33 12  2 44 26  9  1  6 23 34 35 39 22 11 43  3
[26]  4  7 18  5 41 45 37 13 24 30 14 27 42 36 16
 [1] -0.56047565 -0.23017749  1.55870831  0.07050839  0.12928773  1.71506500
 [7]  0.46091619 -1.26506126 -0.68685287 -0.44566196  1.22408175  0.35981384
[13]  0.40077144  0.11068272 -0.55584115  1.78691316  0.49785048 -1.96661711
[19]  0.70135587 -0.47279140 -1.06782365 -0.21797492 -1.02600443 -0.72889125
[25] -0.62503928 -1.68669331  0.83778703  0.15337312 -1.13813698  1.25381494
[31]  0.42646423 -0.29507148  0.89512569  0.87813348  0.82158107  0.68864024
[37]  0.55391765 -0.06191171 -0.30596265 -0.38047099
edd@brad:~/git/rcppannoy(master)$
Owner

eddelbuettel commented Sep 23, 2017

For completeness:

edd@brad:~/git/rcppannoy(master)$ Rscript /tmp/annoycrash.R
 [1]  0 28 17 19 49 32 38 31 47 33 12  2 44 26  9  1  6 23 34 35 39 22 11 43  3
[26]  4  7 18  5 41 45 37 13 24 30 14 27 42 36 16
 [1] -0.56047565 -0.23017749  1.55870831  0.07050839  0.12928773  1.71506500
 [7]  0.46091619 -1.26506126 -0.68685287 -0.44566196  1.22408175  0.35981384
[13]  0.40077144  0.11068272 -0.55584115  1.78691316  0.49785048 -1.96661711
[19]  0.70135587 -0.47279140 -1.06782365 -0.21797492 -1.02600443 -0.72889125
[25] -0.62503928 -1.68669331  0.83778703  0.15337312 -1.13813698  1.25381494
[31]  0.42646423 -0.29507148  0.89512569  0.87813348  0.82158107  0.68864024
[37]  0.55391765 -0.06191171 -0.30596265 -0.38047099
edd@brad:~/git/rcppannoy(master)$
@erikbern

This comment has been minimized.

Show comment
Hide comment
@erikbern

erikbern Sep 24, 2017

weird that it was working before

erikbern commented Sep 24, 2017

weird that it was working before

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 24, 2017

Owner

I carried a bunch of your new tests over but I can only imagine that this one was not covered but what I have.

Owner

eddelbuettel commented Sep 24, 2017

I carried a bunch of your new tests over but I can only imagine that this one was not covered but what I have.

@pommedeterresautee

This comment has been minimized.

Show comment
Hide comment
@pommedeterresautee

pommedeterresautee Sep 25, 2017

@eddelbuettel do you plan to push an update on Cran?

pommedeterresautee commented Sep 25, 2017

@eddelbuettel do you plan to push an update on Cran?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 25, 2017

Owner

Sure, but maybe not immediately. Can you rebuild locally?

Owner

eddelbuettel commented Sep 25, 2017

Sure, but maybe not immediately. Can you rebuild locally?

@pommedeterresautee

This comment has been minimized.

Show comment
Hide comment
@pommedeterresautee

pommedeterresautee Sep 25, 2017

I have rebuilt succesfully, but I want to release a package on Cran depending on RcppAnnoy :-)
Right now, it s quite hard to explain to users to reinstall RcppAnnoy from github if they already installed it...

pommedeterresautee commented Sep 25, 2017

I have rebuilt succesfully, but I want to release a package on Cran depending on RcppAnnoy :-)
Right now, it s quite hard to explain to users to reinstall RcppAnnoy from github if they already installed it...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 25, 2017

Owner

Oh, I see :) In that case I will gladly push a fix.

Owner

eddelbuettel commented Sep 25, 2017

Oh, I see :) In that case I will gladly push a fix.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 26, 2017

Owner

Thanks again for the bug report. RcppAnnoy 0.0.10 is now on CRAN.

Owner

eddelbuettel commented Sep 26, 2017

Thanks again for the bug report. RcppAnnoy 0.0.10 is now on CRAN.

@pommedeterresautee

This comment has been minimized.

Show comment
Hide comment
@pommedeterresautee

pommedeterresautee Sep 26, 2017

Awesome! I ll push mine :-)

pommedeterresautee commented Sep 26, 2017

Awesome! I ll push mine :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment