search

anigmetov / hera

Other
4 stars 3 forks source link

OneTBB support #7

Open mglisse opened 2 years ago

mglisse commented 2 years ago

https://github.com/anigmetov/hera/blob/8fbae1d789b3c9d7e9b079284c85489d8dcd7e65/wasserstein/include/dnn/parallel/tbb.h#L47

In recent versions of TBB, parallel_do is gone. The recommended replacement seems to be parallel_for_each.

anigmetov commented 2 years ago

I fixed this in version 2.0, which I have just pushed. Thank you! Note that the new version is not backward-compatible, sorry. The required change in code using Hera is completely trivial: #include <hera/wasserstein.h> instead of #include . Also CMakeLists.txt can be simplified. I added a paragraph about this in README.

mglisse commented 2 years ago

Thanks! I have to admit that we probably never enabled TBB with Hera, I only noticed it because a grep in gudhi reported this instance in Hera. Apparently we need -DTBB to enable it? (in addition to the usual flags for TBB of course) I don't think we will be able to take advantage of the modern cmake thing because of the weird things we do to pass flags to setup.py, but that's fine. We are probably more impacted by the change of layout of the files, but that should be trivial for us to adapt. I notice that you now have python bindings. Do you intend to make a python package (on pypi, conda)? Or are you expecting the many packages that already ship some bindings for hera to replace theirs with this one? Or is it mostly for your own tests?

anigmetov commented 2 years ago

Honestly, I don't know. The kd-tree code was written by Dmitriy. @mrzv do you think Hera would work with TBB or benefit from it? Kd-tree code requires task_group, but in euclidean-fixed.h it seems to use std::vector in all cases, while it should use dnn::vector (which wraps either tbb::concurrent_vector or std::vector). I mean, to me TBB support seems broken now, and I am not sure we even need it. As for python bindings, I think I will make a pypi package available. The main point of v2.0.0 was not restructuring, but giving users access to the matching computed by wasserstein_dist, to use it in topological optimization. There is a pypi already https://pypi.org/project/hera-tda/, and Brad Nelson asked me to contact him, if I am planning to make my own bindings. Hera is also available (in python) in Dionysus2, of course. It is a pity if you won't be able to take the advantage of target_link_library, but I think that in this case you just need to add hera/include and hera/extern to include directories yourself.

mglisse commented 2 years ago

The main point of v2.0.0 was not restructuring, but giving users access to the matching computed by wasserstein_dist, to use it in topological optimization.

Ah, nice. In Gudhi https://gudhi.inria.fr/python/latest/wasserstein_distance_user.html we currently use POT when we need the matching (parameter matching or enable_autodiff), we will look into enabling it also for Hera.

There is a pypi already https://pypi.org/project/hera-tda/, and Brad Nelson asked me to contact him, if I am planning to make my own bindings. Hera is also available (in python) in Dionysus2, of course.

Yes, there are many packages using Hera, I just didn't know if you considered one to be more official. Now I know :grinning:

mrzv commented 2 years ago

Oh, man, I don't even remember what the TBB code was supposed to do for the k-d tree. It must be using it during the construction, because where else? But that's probably splitting hairs from Hera's point of view. The only interesting use for TBB in Hera would to parallelize the auction algorithm, as far as I'm concerned