Open Jean-Romain opened 4 years ago
Wow... thanks a lot @Jean-Romain !! That's a great input, but I'm a bit embarassed you actually read my C++ code :sweat_smile: ... it is far from optimal and not very DRY.
Most of it was transcribed from pure C++ code I used to create some command line tools. They read the las files from disk using laslib
, and essentially what I did on TreeLS was wrapping R/C++ objects back and forth (e.g. xyz
matrices) using std::vector
since I didn't have to read files from disk anymore. I was aware that this approach, of course, is far from optimal in terms of memory usage, as it brings a lot of overhead.
Your input was perfect in timing, as of v2.0
I included all algorithms and methods I planned since refactoring the package before its 1st CRAN release last year. Now seems like a good time for another deep refactoring, especially targeting the C++
backend. I didn't take the time to properly learn Rcpp
, so I just minimized my workload by making those rather inneficient R/C++ wrappers (pretty much the whole r_interface.cpp
file).
On top of that, developing TreeLS has been slow since I do it as a side project on my spare time. I'm getting back to the academy as of this year though, and I'll be working with LiDAR again, although it's GEDI data this time, BUT it will be great to bring TLS into the mixture, so I might be able to include TreeLS' development as part of my research. So lets cheer for it!
Well... enough blabber, let's talk business :point_down:
As already said, I am well aware of the memory issues, and this is especially valid for all methods relying on knn
, which are very memory hungry. The most important thing on a next code refactor will be replacing those dirty wrappers with proper references/pointers to Rcpp
data structures, but also, ideally the knn
search should be done entirely in C++. Is there any Rcpp
compatible API you recommend for fast knn search?
Regarding ...I don't think your code can handle big TLS datasets..., I'd say those methods get the job done for "normal" forest inventory plots and single trees. They should be able to perform on clouds with up to 30 million points on a 16GB RAM. Of course it can be much better, but for now any really large datasets should be processed in chunks, and most forest inventory plots should be processed just fine so far on computers with at least 8GB of RAM and/or after sampling the point clouds. By the way, most TreeLS methods should be compatible with lidR::catalog
, but I haven't explored it, so any remarks you may have on that will be great.
Very interesting. Is this spatial index based on octrees, voxels or something else? Any references you can share?
Awesome!!! Being able to access lidR's functions straight from C++ is a HUGE help. That will also be really helpful for future implementations of even more memory hungry applications, like tree QSMs. Do you have (or will provide) docs for the low-level API?
TreeLS' C++ code sure needs some refurbishing. All your inputs here were really helpful and if you want to submit any PRs you're more than welcome (or anyone else reading this and thinking they might be able to contribute). I can't jump straight into refactoring TreeLS again as I have too much on my plate right now, but I'll try to be slow and steady at least. When do you plan on releasing lidR 3.1? If there are any related features you want a second opinion or extra testing let me know and I'll be glad to help. As soon as I start digging into lidR's code again I'll have questions to throw at you, be sure of that. Thanks again!
but I'm a bit embarassed you actually read my C++ code
Make it close source! Of course I read (some piece of) your code, I'm following your work from the beginning :wink:.
Is there any Rcpp compatible API you recommend for fast knn search?
Well... mine?!? It is fully optimized for LiDAR data. If you want another I can't help sorry.
Very interesting. Is this spatial index based on octrees, voxels or something else? Any references you can share?
No octree. Historically I used quad trees but I moved to grid-based index. ALS data are evenly spread and quad tree introduce a lot of complexity for nothing. Moving to a grid-based index allowed to get a x2 speed-up on all queries and thus to many algorithms. See changelog v2.2.0. In lidR knn search is faster than any other knn search in R (FANN
, RANN
, nabor
) because it is specialized to ALS. The new extension to TLS is only an extension of the 2D grid-based index to a 3D voxel-based index. I didn't benchmark it yet against nabor
but from my early test shown above it looks good.
Any references you can share?
The source code isn't complex and is commented (but could be more commented).
Do you have (or will provide) docs for the low-level API?
Not yet. This is why I said to do not hesitate to ask me. With your feed back the API may change a bit. I'll document it later if I find how to document C++ classes in R. Anyway the code is simple and there is only few public members.
if you want to submit any PRs you're more than welcome
Actually I forked your repos to make a real test but your code was more complex than expected.
When do you plan on releasing lidR 3.1?
Not before Jan 2021.
If there are any related features you want a second opinion or extra testing let me know and I'll be glad to help.
Just use my class (if you want) and if you encounter difficulties or you think there is a missing feature we can discuss. The class is first designed for my own use so having a feed back from a real user may help to improve.
I just added comments to all public members that can act like a simple source of documentation
This issue gonna be long. I'm presenting new C++ feature in
lidR
that will strongly benefit toTreeLS
both in term of memory and speed. I'll start explaining some memory issues inTreeLS
, then explain you how to improve that by linking tolidR
.Memory and speed issues
I'm using
fastPointMetrics
withknn = 10
as an example but for what I've seen it may apply to other functions.fastPointMetrics
is not as fast as it could be mainly because inefficient memory management at different level. TLS data are huge, thus making a copy of point clouds must be performed carefully. In your case I can see many copies in the code. By copy I mean a block of memory the size of the point cloudHere we have many copies:
las %>% las2xyz
creates two copies.nabor::knn
creates 6 copies fork = 10
. It allocatesk x n
matrix ofint
+ ak x m
matrix ofdouble
which mean approx 5 times the memory of thexyz
coordinates + the spatial index.Here at least 2 more copies
Here one again.
I may have missed some of the copies. I counted at least 12 copies. Some are temporary, some are persistent for the whole duration of
fastPointMetrics
. Anyway a copy has a cost in computation time and memory usage and I don't thing your code can handle big TLS datasets and is slower than it could be.How lidR manages such case
lidR
has its own spatial index. This allows to avoid copies usingpkg::knn
at R level working only at c++ level efficiently with a single copy avoiding any type casting such asdata.frame
tomatrix
. Sadly this spatial index was optimized for ALS data and thus was slow on TLS data. The computational cost offastPointMetric
being the eigen decomposition we can compare withsegment_shape
that is almost equivalent in term of computation. Your function is faster.New in lidR 3.1.0 - high-level API
In lidR 3.1.0 the spatial index can be adapted and extended to TLS data. Below the high level API.
LAS
objects can now be tagged with an acquisition source. We can see the huge seed up. It is faster thanfastPointMetrics
and uses much fewer memory.This applies to the generic
point_metrics
function. Withpoint_metrics
you can fully reproduce yourfastPointMetric
only at the R level being memory optimized (a single copy) and approximately as fastNew in lidR 3.1.0 - low-level API
But you can do even better. The spatial index is now available and you can
LinkingTo
lidR.point_metrics
is optimized but make R call internally. This kills the performance. We can use the spatial index in lidR to do that fully at C++ level.Let try it. The code is simpler, faster and memory optimized.
You can also make query in arbitrary shape with
lookup
that is templatedConclusion
All of this is experimental. The API is subject to modifications until release but you can yet considerably improve
TreeLS
and make it at least as fast (probably faster) but less memory greedy and you can simplify drastically your code. If you have any question do not hesitate. I'd like refine the code with your feed back. It passed the 1000+ unit tests of lidR so I guess it is safe but we may still encounter some trouble so your feed back is welcome.Cheers.