-
assigned issue to
Remove Boost
After bimap is added, update each module to use std::shared_ptr.
Fully removing Boost will also require the implementation of an R*-tree or another spatial data structure to replace the boost::geometry
code in registration/brute_force.h
.
Comments (13)
-
reporter -
reporter Hey Will, since hipchat is down (cause of course it is) let me know if you have any questions. Sorry I didn't get a chance to talk today, had to run to get my car serviced.
-
reporter - marked as major
-
reporter Partially addressed in PR #84, but not quite done
-
Related issue:
#104 -
- removed responsible
- edited description
-
-
assigned issue to
-
assigned issue to
-
The R-tree component of this is related to Issue #168. If the plan is to replace brute_force entirely with a different registration algorithm then this point is moot. However, I suspect that we won't be able to come to a consensus on the best registration toolkit in the short term.
In the interest of getting Boost out in the near term, I propose replacing the Boost R-tree with the libspatialindex implementation. That code is old and well tested, it's licensed under the MIT license so we can use it, and it would be easy to maintain a mirror and add it as a submodule. That would address our primary concerns with Boost being a pain to install and maintain for freud users on systems that don't come with Boost. The primary issue with libspatialindex is that it isn't thread safe, but currently we're not making use of TBB anywhere in MatchEnv or brute_force so I don't think that's an issue.
@harperic @bdice @erteich @the_real_pdodd any thoughts? We could always implement our own R-tree, I've implemented similar data structures in the past and it could be done in a couple days. But getting a clean, bug-free implementation might take a bit longer, and it seems like a waste of time for such a standard tool with plenty of existing implementations.
-
I'm supportive of libspatialindex as a replacement. I would not advocate for us to write our own solution, since we already have submodule dependencies and this seems to be a clear alternative.
-
reporter libspatialindex seems like a fine solution to me.
-
It turned out that libspatialindex was too heavy. @vramasub and I considered replacing the boost R*-tree with a KD-tree, for which there are many header-only C++ implementations, but I decided against using any spatial indexing scheme. This may introduce minor performance regressions, to be determined.
This issue will be essentially resolved after the two PRs for
shared_ptr
and registration are merged. Then all that will remain is a cleanup task to remove Boost from CMake files, documentation, etc. -
- changed status to closed
Boost has been removed.
-
- changed version to 0.9
- Log in to comment