glotzerlab / freud

Powerful, efficient particle trajectory analysis in scientific Python.
https://freud.readthedocs.io
BSD 3-Clause "New" or "Revised" License
276 stars 49 forks source link

Remove Boost #105

Closed bdice closed 6 years ago

bdice commented 8 years ago

Original report by Eric Harper (Bitbucket: harperic, GitHub: harperic).


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.

bdice commented 6 years ago

Original comment by Bradley Dice (Bitbucket: bdice, GitHub: bdice).


Boost has been removed.

bdice commented 6 years ago

Original comment by Bradley Dice (Bitbucket: bdice, GitHub: bdice).


It turned out that libspatialindex was too heavy. @vyasr 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.

bdice commented 6 years ago

Original comment by Eric Harper (Bitbucket: harperic, GitHub: harperic).


libspatialindex seems like a fine solution to me.

bdice commented 6 years ago

Original comment by Bradley Dice (Bitbucket: bdice, GitHub: bdice).


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.

bdice commented 6 years ago

Original comment by Vyas Ramasubramani (Bitbucket: vramasub, GitHub: vyasr).


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.

bdice commented 6 years ago

Original comment by Bradley Dice (Bitbucket: bdice, GitHub: bdice).


Related issue: #104

bdice commented 7 years ago

Original comment by Eric Harper (Bitbucket: harperic, GitHub: harperic).


Partially addressed in PR #84, but not quite done

bdice commented 8 years ago

Original comment by Eric Harper (Bitbucket: harperic, GitHub: harperic).


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.