miss setup.py - Githubissues

koide3 / small_gicp

Efficient and parallel algorithms for point cloud registration [C++, Python]

MIT License

406 stars 51 forks source link

miss setup.py #24

Closed mcmingchang closed 5 months ago

mcmingchang commented 6 months ago

miss setup.py

valgur commented 6 months ago

Please update your setuptools and/or pip. setup.py is no longer required for Python packages.

koide3 commented 6 months ago

I'll update README.

valgur commented 6 months ago

A minimal

from setuptools import setup

setup()

can still be included in the repo for backwards compatibility to keep things simple, as suggested by setuptools itself: https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html

koide3 commented 6 months ago

Thanks for the info. I think it's OK to leave it in the current form and drop the old way.

mcmingchang commented 6 months ago

Under the same point cloud and parameters, using Ubuntu's Python takes 30ms to run. When I directly called the relevant code on VS2022, I found that OMP version 2.0 cannot be called, and adding -openmp: llvm took 60ms. I suspect it is a version issue with OMP. I am unable to build the Python version on Windows now. Can you help me see how to build a Python version of Win or improve its running speed on VS2022? If tbb is on when compiling Python packages, will the calculation speed be faster?

koide3 commented 6 months ago

The python interfaces uses only OpenMP and thus building with TBB wouldn't help. I don't have a windows environment for now, and it would be appreciated if you could help investigate the issue. When you increase the number of threads, do you observe that the CPU usage increases as well?

mcmingchang commented 6 months ago

Yes, CPU usage is also increasing

koide3 commented 6 months ago

Can you check if the build mode is set to RELEASE?

mcmingchang commented 6 months ago

Confirm that it is set to RELEASE.

koide3 commented 6 months ago

Hmm, it might stem from a difference in the OpenMP implementations in linux/windows. Could you take a timing profile to see which part of the code causing the difference of processing time?

valgur commented 6 months ago

It might be worth testing with the LLVM implementation of OpenMP instead of the native MSVC one. It's not super easy to set up, unfortunately. Conan is getting support for it soon-ish: https://github.com/conan-io/conan-center-index/pull/22353.

I might be able to do some benchmarking, but I only have a VM-based setup of Windows, so I don't know how representative it will be.

Edit: small_gicp cannot be built with MSVC OpenMP at all in my tests. It fails with:

error C3016: 'i': index variable in OpenMP 'for' statement must have signed integral type
error C7660: 'task': requires '-openmp:llvm' command line option

koide3 commented 5 months ago

When I tried MSVC last time, both the errors were resolved by adding -openmp:llvm. But, I'm not sure how it affects the performance. Because I cannot use the MSVC community edition in my workplace and don't have a pro edition, I'll try to do some tests in the next weekend at home.

koide3 commented 5 months ago

I slightly updated the code for windows https://github.com/koide3/small_gicp/pull/40. I did a quick benchmark and observed good multi-thread scalability with /openmp:llvm. Although I didn't do a comprehensive benchmark, I think it at least works reasonably.