search

crflynn / skgrf

scikit-learn compatible Python bindings for grf (generalized random forests) C++ random forest library
https://skgrf.readthedocs.io/en/stable/
GNU General Public License v3.0
30 stars 6 forks source link

Move cpp extern refs into build sources #62

Closed crflynn closed 3 years ago

erikcs commented 3 years ago

Another tiny build related comment: setup.py does not define an optimization level flag, so the package will compile with the system's Python default. The CI builds it with -O2, my mac laptop happened to build it with -O3:

Screen Shot 2021-06-30 at 20 43 26

Screen Shot 2021-06-30 at 20 44 05

I don't know what the convention for Python packages is, and I guess it is not a big issue, just maybe worth making a note of, in case someone manages to compile with -O0 and complains (sklearn apparently sets it to -O3: https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/tree/setup.py#L17)

crflynn commented 3 years ago

https://www.spec.org/cpu2017/flags/gcc.2018-02-16.html#user_Olevel-gcc Ah so ideally we would want to set to -O3 to have the highest level of optimization by the compiler.

erikcs commented 3 years ago

https://www.spec.org/cpu2017/flags/gcc.2018-02-16.html#user_Olevel-gcc Ah so ideally we would want to set to -O3 to have the highest level of optimization by the compiler.

Setting the flag here is just convenient for standardization I think - I was just thinking ahead about some possible minor headaches, say this package's binary is distributed through different "vendors" like conda-forge / pip / etc - it would be awkward if conda-users got the package binaries compiled with -O3 and the pip users with -O0 : P (but again, I am not up to speed on the Python ecosystem's distribution of compiled packages and their conventions)