Ready for release? - Githubissues

Jean-Romain commented 6 years ago

The package is almost ready to be released on CRAN

[x] C++ code passes valgrid tests
[x] Package passes R CMD check
[x] Modification of original C++ code to enable user abortion if the computation time is too long. f729fdb
[x] Remove std::cout code that is not allowed in R. Could be replaced by Rcpp::Rcout eventually. f729fdb
[ ] Check the documentation (spelling, correctness)
[ ] add a longer description in file DESCRIPTION in tag Description:

@jianboqi could you please look at the two unchecked boxes? Thanks.

Also 3 important points to discuss:

the package has only one function. Is there some other features that could be exposed to R?
openMP? I can enable the support of openMP but to be honest I need to learn first. So is it really useful? Do you have benchmark? What is the gain? In other words is it really cost-effective to me to spend hours on this question?
progress bar. You commented the progress bar. I understand why. You don't actually know the progress because the number of iteration is not fixed. The algo can finish after 2 iterations or 100. Do you have a idea how we could estimate the progress? I think it is important because we are talking about systematically long computations

jianboqi commented 6 years ago

Hello, @Jean-Romain Thank you very much for your work. I will read the documentation and write the Description part. For the 3 points:

Another feature that can be useful is the exporting of the cloth. do_filtering function has a parameter exportCloth, which could output the cloth particle. This is not very important feature, because most of the user will not have interests on this. I mention this only because the cloudcompare plugin has the option to export the cloth particle. void CSF::do_filtering(std::vector& groundIndexes, std::vector& offGroundIndexes, bool exportCloth)
The openMP is an important feature the accelerate the filtering process, espeically when the lidar point cloud is large (in terms of extent). When their are no openMP, it will be slower. For small point cloud, this is not significant.
For the progress bar. The solution maybe set the maimum to 500. if the progress stops before it reaches 500, we can set the progress bar 100% immeadiately. This is only solution I can think of now. Is this OK?

Jean-Romain commented 6 years ago

Well actually the package is not ready to be released. There are several things to fix in your c++ code. I compiled the package on CRAN where the checks are more severe than on my config:

One important point to fix is:

point_cloud.h:47:9: warning: ISO C++ prohibits anonymous structs [-Wpedantic]

The other issues are less important but should be fixed as well (skip those relative to openmp for the moment)

https://win-builder.r-project.org/h1L86v5FcFaN/00install.out

The logs (available 72h) https://win-builder.r-project.org/h1L86v5FcFaN/

Jean-Romain commented 6 years ago

I enabled the support of OpenMP (at least on my machine, I can't swear it is a valid way to enable OpenMP on CRAN but it does not matter yet). OpenMP does work because I benchmarked some parallelized test functions. So OpenMP is correclty linked. However I found zero gain runing the CSF on a 500 x 500 m files with 4 pts/m². So I suggest either:

provide me a reproducible example (with data) on which I can see the benefit of openMP (optionally using the original lib and a binary I can run in a terminal). This way I will be able to test if the issue comes from the package

or

remove all the openmp stuff. I will probably make sleepless nights by releasing a package with openMP support on CRAN. So if we can remove that it will save me a lot of pain :wink:

Jean-Romain commented 6 years ago

I fixed non ISO code of anonymous structs. You should consider doing the same in the original source code.

Jean-Romain commented 6 years ago

I fixed constant warnings. The package is almost perfect regarding CRAN requirements. One warning is remaining. You defined twice ERROR.

* installing *source* package 'RCSF' ...
** libs

*** arch - i386
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c CSF.cpp -o CSF.o
In file included from D:/RCompile/recent/R/include/R.h:91:0,
                 from d:/RCompile/CRANpkg/lib/3.6/Rcpp/include/Rcpp/r/headers.h:52,
                 from d:/RCompile/CRANpkg/lib/3.6/Rcpp/include/RcppCommon.h:29,
                 from d:/RCompile/CRANpkg/lib/3.6/Rcpp/include/Rcpp.h:27,
                 from Progress.h:4,
                 from CSF.cpp:27:
D:/RCompile/recent/R/include/R_ext/RS.h:55:0: warning: "ERROR" redefined
 #define ERROR   ),error(R_problem_buf);}
 ^
In file included from D:/Compiler/gcc-4.9.3/mingw_32/i686-w64-mingw32/include/windows.h:71:0,
                 from Cloth.h:44,
                 from CSF.cpp:23:
D:/Compiler/gcc-4.9.3/mingw_32/i686-w64-mingw32/include/wingdi.h:75:0: note: this is the location of the previous definition
 #define ERROR 0
 ^
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Cloth.cpp -o Cloth.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Constraint.cpp -o Constraint.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Particle.cpp -o Particle.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Progress.cpp -o Progress.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c R_CSF.cpp -o R_CSF.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Rasterization.cpp -o Rasterization.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c RcppExports.cpp -o RcppExports.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c XYZReader.cpp -o XYZReader.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c c2cdist.cpp -o c2cdist.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++  -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c point_cloud.cpp -o point_cloud.o
d:/Compiler/gcc-4.9.3/mingw_32/bin/g++ -shared -s -static-libgcc -o RCSF.dll tmp.def CSF.o Cloth.o Constraint.o Particle.o Progress.o R_CSF.o Rasterization.o RcppExports.o XYZReader.o c2cdist.o point_cloud.o -fopenmp -Ld:/Compiler/gcc-4.9.3/local330/lib/i386 -Ld:/Compiler/gcc-4.9.3/local330/lib -LD:/RCompile/recent/R/bin/i386 -lR
installing to d:/RCompile/CRANguest/R-devel/lib/RCSF/libs/i386

*** arch - x64
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c CSF.cpp -o CSF.o
In file included from D:/RCompile/recent/R/include/R.h:91:0,
                 from d:/RCompile/CRANpkg/lib/3.6/Rcpp/include/Rcpp/r/headers.h:52,
                 from d:/RCompile/CRANpkg/lib/3.6/Rcpp/include/RcppCommon.h:29,
                 from d:/RCompile/CRANpkg/lib/3.6/Rcpp/include/Rcpp.h:27,
                 from Progress.h:4,
                 from CSF.cpp:27:
D:/RCompile/recent/R/include/R_ext/RS.h:55:0: warning: "ERROR" redefined
 #define ERROR   ),error(R_problem_buf);}
 ^
In file included from D:/Compiler/gcc-4.9.3/mingw_64/x86_64-w64-mingw32/include/windows.h:71:0,
                 from Cloth.h:44,
                 from CSF.cpp:23:
D:/Compiler/gcc-4.9.3/mingw_64/x86_64-w64-mingw32/include/wingdi.h:75:0: note: this is the location of the previous definition
 #define ERROR 0
 ^
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Cloth.cpp -o Cloth.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Constraint.cpp -o Constraint.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Particle.cpp -o Particle.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Progress.cpp -o Progress.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c R_CSF.cpp -o R_CSF.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c Rasterization.cpp -o Rasterization.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c RcppExports.cpp -o RcppExports.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c XYZReader.cpp -o XYZReader.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c c2cdist.cpp -o c2cdist.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -I"D:/RCompile/recent/R/include" -DNDEBUG  -I"d:/RCompile/CRANpkg/lib/3.6/Rcpp/include"   -I"d:/Compiler/gcc-4.9.3/local330/include"  -fopenmp   -pedantic -O2 -Wall  -mtune=core2 -c point_cloud.cpp -o point_cloud.o
d:/Compiler/gcc-4.9.3/mingw_64/bin/g++ -m64 -shared -s -static-libgcc -o RCSF.dll tmp.def CSF.o Cloth.o Constraint.o Particle.o Progress.o R_CSF.o Rasterization.o RcppExports.o XYZReader.o c2cdist.o point_cloud.o -fopenmp -Ld:/Compiler/gcc-4.9.3/local330/lib/x64 -Ld:/Compiler/gcc-4.9.3/local330/lib -LD:/RCompile/recent/R/bin/x64 -lR
installing to d:/RCompile/CRANguest/R-devel/lib/RCSF/libs/x64
** R
** data
*** moving datasets to lazyload DB
** byte-compile and prepare package for lazy loading
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded
*** arch - i386
*** arch - x64
* MD5 sums
packaged installation of 'RCSF' as RCSF_0.1.0.zip
* DONE (RCSF)
In R CMD INSTALL

Jean-Romain commented 6 years ago

Everything fixed by removing #include <windows.h> fa420e7d5.

Jean-Romain commented 6 years ago

I probably miss tested the code with valgrind. Using the example dataset the CFS return randomly 15457 or 15458 ground points instead of 15459. This come with no doubt from an error of memory allocation.

Edit: actually my test with valgrind was made before to support openMP. Now openMP works, valgrind shows memory error. My opinion is to remove all omp stuff.

==5960== Syscall param sched_setaffinity(mask) points to unaddressable byte(s)
==5960==    at 0x57804D9: syscall (syscall.S:38)
==5960==    by 0xDEAF4D7: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDEA3D2F: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE947AE: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE8B2A3: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE82ED1: __kmpc_fork_call (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xD94DB06: CSF::setPointCloud(std::__1::vector<csf::Point, std::__1::allocator<csf::Point> >) (CSF.cpp:52)
==5960==    by 0xD95706C: R_CSF(Rcpp::DataFrame_Impl<Rcpp::PreserveStorage>, bool, double, double, int, int, double) (R_CSF.cpp:54)
==5960==    by 0xD95D0A0: _RCSF_R_CSF (RcppExports.cpp:21)
==5960==    by 0x4F0C7A9: ??? (in /usr/lib/R/lib/libR.so)
==5960==    by 0x4F0CC9B: ??? (in /usr/lib/R/lib/libR.so)
==5960==    by 0x4F4A20C: Rf_eval (in /usr/lib/R/lib/libR.so)
==5960==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==5960== 
══ testthat results  ═══════════════════════════════════════════════════════════
OK: 1 SKIPPED: 0 FAILED: 0
> 
==5960== 
==5960== HEAP SUMMARY:
==5960==     in use at exit: 51,262,296 bytes in 23,831 blocks
==5960==   total heap usage: 664,562 allocs, 640,731 frees, 168,685,058 bytes allocated
==5960== 
==5960== 112 bytes in 1 blocks are definitely lost in loss record 58 of 2,549
==5960==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5960==    by 0xDE6D03C: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE6D13A: ___kmp_allocate (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE8D454: __kmp_fork_call (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE82F4E: __kmpc_fork_call (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xD94DB06: CSF::setPointCloud(std::__1::vector<csf::Point, std::__1::allocator<csf::Point> >) (CSF.cpp:52)
==5960==    by 0xD95706C: R_CSF(Rcpp::DataFrame_Impl<Rcpp::PreserveStorage>, bool, double, double, int, int, double) (R_CSF.cpp:54)
==5960==    by 0xD95D0A0: _RCSF_R_CSF (RcppExports.cpp:21)
==5960==    by 0x4F0C7A9: ??? (in /usr/lib/R/lib/libR.so)
==5960==    by 0x4F0CC9B: ??? (in /usr/lib/R/lib/libR.so)
==5960==    by 0x4F4A20C: Rf_eval (in /usr/lib/R/lib/libR.so)
==5960==    by 0x4F4CCAD: ??? (in /usr/lib/R/lib/libR.so)
==5960== 
==5960== 416 bytes in 1 blocks are definitely lost in loss record 133 of 2,549
==5960==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5960==    by 0xDE6D03C: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE6D13A: ___kmp_allocate (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDEA82B4: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDEA85C1: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDEC3D91: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE953E6: ??? (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE8E567: __kmp_fork_call (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xDE82F4E: __kmpc_fork_call (in /usr/lib/x86_64-linux-gnu/libomp.so.5)
==5960==    by 0xD94DB06: CSF::setPointCloud(std::__1::vector<csf::Point, std::__1::allocator<csf::Point> >) (CSF.cpp:52)
==5960==    by 0xD95706C: R_CSF(Rcpp::DataFrame_Impl<Rcpp::PreserveStorage>, bool, double, double, int, int, double) (R_CSF.cpp:54)
==5960==    by 0xD95D0A0: _RCSF_R_CSF (RcppExports.cpp:21)
==5960== 
==5960== LEAK SUMMARY:
==5960==    definitely lost: 528 bytes in 2 blocks
==5960==    indirectly lost: 0 bytes in 0 blocks
==5960==      possibly lost: 0 bytes in 0 blocks
==5960==    still reachable: 51,261,768 bytes in 23,829 blocks
==5960==         suppressed: 0 bytes in 0 blocks
==5960== Reachable blocks (those to which a pointer was found) are not shown.
==5960== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==5960== 
==5960== For counts of detected and suppressed errors, rerun with: -v
==5960== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

jianboqi commented 6 years ago

Hello @Jean-Romain The performance gain of openMP can be simply tested by increasing the cloth_resolution. Since the time performance is mainly influenced by the number of cloth particles in the cloth not the number of points in the point cloud. If you makes the cloth resolution higer, the computation time will increase. OpenMP may be needed. However, if it is difficult to enable OpenMP in CRAN or it will cause some problems, I wish to remove all the omp stuff. Maybe single core is enough in most of the cases. And also this may be easier for you to submit the package to CRAN. Additionally, I already added a longer DESCRIPATION. Thank you for your nice work!

Jean-Romain commented 6 years ago

However, if it is difficult to enable OpenMP in CRAN or it will cause some problems, I wish to remove all the omp stuff.

I don't think it is actually that hard (I think what I did is correct). However there are memory leaks in the parallelized version when openMP is enabled leading to random outputs. You must fix them (this time I'm not going to fix it, this is beyond my knowledge, I have never used open MP before) or remove all the omp stuff.

jianboqi commented 6 years ago

The random output is not caused by memory leaks. It is simply a problem of parallel computing. When openMP is enabled, the movement of cloth paritcles is simulated parallelly. The final shape of cloth may be a little different from simulation to simulation. Thus, when using the cloth to classify point cloud, some points may be classified into ground or non-ground. The output may be shows some random. As for the valgrind test, this may not be caused by OpenMP. Is it related to setPointCloud ? I am not very sure.

Jean-Romain commented 6 years ago

Maybe the slightly random nature of the ouput is not the fact of the memory leak but anyway there is a memory leak. This will not pass on CRAN. valgrind outputs are hard to understand. I'm now relatively comfortable with the regular issues but here it is related to openMP or wrapped into openMP stuff and it is definitively not a "regular case" to me.

And yes, It happens only l52 of CSF.cpp. Maybe I can test without the parallelization of this part which is not of major importance.

jianboqi commented 6 years ago

I don't know how to use valgrind. Could you please try to test it without openMP. If the problem is caused by OpenMP, I prefer to remove all codes related to OpenMP, Since I am also not a expert in using OpenMP. In the code, I just use some very basic OpenMP instructions to accelerate the program. Thank you!

Jean-Romain commented 6 years ago

I already did (see above). When openmp was not linked there was no memory leak.

Jean-Romain commented 6 years ago

I commented the line 52 in CSF.cpp. I got new memory leaks in others places. Please comment every single line that contains an openmp statement (including the includes). That will definitively close the question.

jianboqi commented 6 years ago

Last question, I just looked some document about valgrind, it says the problem " still reachable: 51,261,768 bytes in 23,829 blocks" may not be a prolblem. http://valgrind.org/docs/manual/faq.html#faq.deflost

“still reachable” means your program is probably ok — it didn’t free some memory it could have. This is quite common and often reasonable. Don’t use — show-reachable=yes if you don’t want to see these reports. Maybe you can simply disable this message. If this is not the reason, I will comment all the OpenMP codes.

jianboqi commented 6 years ago

I didnot find the problem of "definitely lost", so I will disable all the openMP in the code.

Jean-Romain commented 6 years ago

The CRAN manage 10000 packages. They don't (understandably) care if the issues are false positive or not. This is why the memory leak will be a problem anyway. So we have fix it by any mean :wink:

jianboqi commented 6 years ago

OK, I removed all the openMP related codes.

Jean-Romain commented 6 years ago

Great. I also shorten the description to fit better with a classical package description. I think everything looks good now.

r-lidar / RCSF

Ready for release? #4