How should we bundle libopenpiv with python?

ErichZimmer commented 2 years ago

To build libopenpiv from python, there are several ways. One way is the way I did it with my piv filters test using setuptools to compile everything into a .pyd or .so extension. This way may not be optimal, but it may be simpler and doesn't require vcpkg (possible drawback from not using cmake).

Another option is to compile this repository with vcpkg and link the wrapper to the dll/so in a similar manner to pyfftw. To install from source (unrecommended if a precompiled wheel is already made), you would have to compile openpiv with vcpkg and listen to what the output of setup.py to make sure everything is good and proceed as normal.

timdewhirst commented 2 years ago

From some cursory googling it looks like using setup tools and have it call cmake is the way to go; there are some examples/notes:

Ideally, as the C++ side already builds I would like to avoid having two parallel build systems for the core lib.

It's probably worth noting the role of vcpkg: vcpkg is a simple way of getting up-to-date dependencies without having to use submodules for each dependency; the downside is that vcpkg is tightly coupled to cmake. openpivcore's cmake & vcpkg are currently setup to produce a library with no external dependencies other than the required system dependencies and stdc++:

% otool -L libopenpivcore.dylib 
libopenpivcore.dylib:
    @rpath/libopenpivcore.dylib (compatibility version 0.0.0, current version 0.0.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1311.100.3)
    /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1300.23.0)

As such, I think it would make sense to build libopenpivcore using cmake, build the pybind11 code using cmake or setup tools and drive the build and package process using setup tools for the python bindings.

I've not thought this through fully yet, but I think that's the approach that I would like to take/seems most sensible.

alexlib commented 2 years ago

note that Windows is an issue https://github.com/sizmailov/pyxmolpp2/issues/54

ErichZimmer commented 2 years ago

I'm having quite a "fun" time linking openpivcore.lib to my very simple wrapper since I'm still quite new to dynamic libraries (I haven't even started computer science 1 yet lol). Perhaps, someone can walk me through an example of linking examples/process but with OpenPIV-c++ already compiled? This might help me get a little farther than the cmake tutorials I have been going through for the last few hours.

ErichZimmer commented 2 years ago

@timdewhirst Cmake executed through setuptools seems to be the way to go, as you proposed. Currently, setuptools and distutils ignore some c++ flags, like setting std to c++17. However, I'm quite new to cmake so I'll spend my off time learning it. C and c++ can be so entertaining at times 😅

timdewhirst commented 2 years ago

@timdewhirst Cmake executed through setuptools seems to be the way to go, as you proposed. Currently, setuptools and distutils ignore some c++ flags, like setting std to c++17. However, I'm quite new to cmake so I'll spend my off time learning it. C and c++ can be so entertaining at times 😅

I agree - it's a steep learning curve, for sure. If there are some specific areas you would like to focus on/need some understanding, I'm happy to put together some notes. I appreciate your time in looking at this project, and also that the set of technologies involved is broad and complicated!

timdewhirst commented 2 years ago

I'm having quite a "fun" time linking openpivcore.lib to my very simple wrapper since I'm still quite new to dynamic libraries (I haven't even started computer science 1 yet lol). Perhaps, someone can walk me through an example of linking examples/process but with OpenPIV-c++ already compiled? This might help me get a little farther than the cmake tutorials I have been going through for the last few hours.

I can probably do that; I think you may be correct that windows will require an export lib - I'll double check.

ErichZimmer commented 2 years ago

For some reason, cmake is having trouble locating openpivcore.lib, even when hard coding the path to said file. Other than that, everything seems to be working (e.g. no errors). For compiling the wrapper, I setup a folder like this: openpiv base folder openpiv/openpiv-c--qt folder with openpiv-c--qt openpiv/openpiv-c--qt/build folder with cmake build openpiv/openpiv-python-c++ python wrapper with additional preprocessing filters openpiv/processor.cpp wrapper for openpivcore main functions openpiv/threads.cpp multi threading openpiv/CMakeList.txt to build .pyd extension of processor.cpp When the wrapper conpiles into a .pyd extension, it is then moved to openpiv/openpiv-python-c++/process where the __init.py__ file can locate it for error checking and other goodies.

On a side note, is there a way to setup a tree structure in GitHub comments? Google didn't help too much.

ErichZimmer commented 2 years ago

Everything is now working, but I still can't manage to get find_package() to work without using PATHS. Other than that, I'll make my wrapper public so you can inspect and enhance.

timdewhirst commented 2 years ago

Everything is now working, but I still can't manage to get find_package() to work without using PATHS. Other than that, I'll make my wrapper public so you can inspect and enhance.

awesome!

ErichZimmer commented 2 years ago

pyprocketfft_vs_openpivcore

Does this error plot seem right? It appears that the FFT algorithm used in OpenPIV-c++ is more suitable than SciPy's pocketFFT. It is nearly 75% faster too :D

ErichZimmer commented 2 years ago

I did some more testing and profiling, and it appears that my CPU has only 1 core, but 4 threads. Yikes!

timdewhirst commented 2 years ago

This is great - lovely to see some comparative analysis being done! The above error plot could be due to a number of factors: difference in FFT implementation, difference in three point estimator implementation, image conversion/bit depth errors, ...

Do you have some details on what the input image format was?

Performace: this is a large and tricky subject... things like thread count aren't necessarily bad but you need to have a solid understanding of your computer's hardware topology. e.g. my laptop is currently an apple M1 pro with 8 cores, but interestingly they're not all equal - I have 6 big and 2 little (or power efficient) - however from a portable C++ point of view they're all treated equally. I also have the advantage of having much better memory bandwidth that a traditional intel based system.

Optimization is vital - modern optimizing compilers are amazing things and can radically improve performance.

The simple process app is designed to use (available cores - 1) for processing, so if your CPU reports four logical cores (whether real of doubled thanks to hyperthreading), three will be used. But for performance you need to think of the computer as a whole - what other processes are running, how will the OS time slice, etc

One approach I often take to get a relative idea of performance is to minimize the non-processing overheads e.g. image loading, and then using an extreme processing regime e.g. a 2x2 grid with 64x64 interrogation areas to force computational costs to the front. This coupled with making sure the test host is as idle as possible usually gives a reasonable indication of relative performance.

ErichZimmer commented 2 years ago

The images are 256x256px in bmp format (due to compatibility issues with my synthetic image generator and other PIV softwares). The particles are modeled on a gaussian function and with gaussian white noise at 0.001 n/px

For the processing of images, the images are first loaded using imageio on python side. The images are then passed to c++ side and copied into core::gf_image for the cross-correlation. The correlation matrix is then copied into a 3D numpy array and returned back to python side. For the subpixel estimation, I used openpiv-python because I was lazy today. So, the "main" difference might simply be the algorithm being more suitable for PIV. On a side note, your algorithm has smaller errors than openpiv-python's normalized linear correlation while being over 8 times faster on one thread. I wonder how normalized correlation would work for the c++ side and if it's worth the computational costs.

On the structure of the wrapper, all main functions are separable. That means that there is a function for the correlation, and a function for the subpixel estimation. I did this to allow for ensemble correlation and multi-frame correlation for some particularly nasty image sets. However, this caused some unecessary copying of data due to my lack of proficiency in c++.

ErichZimmer commented 2 years ago

Here is a quick benchmark on a 512x512px image with 32x32px interrogation window size and 30x30px (2x2px step) while using one thread (best performance on my laptop).

Cross-correlation:

openpiv-python: 22.1 second average
openpiv-python-c++: 8.2 second average
subpixel estimation:
openpiv-python: 2.5 second average
openpiv-python-c++: 8.6 second average (also used quite a bit more RAM)

It looks like I need to work a little bit more on the subpixel estimation function as I pretty much slapped it together.

timdewhirst commented 2 years ago

would it be possible to attach the test images?

ErichZimmer commented 2 years ago

Here is the link to the repository. https://github.com/ErichZimmer/OpenPIV-Python-cxx

For the images, I'll upload them in a test folder in the repository.

ErichZimmer commented 2 years ago

@timdewhirst I added some test images that I used for the error testing. Velocity magnitude starts at 0 and maxes out at 2 with a step of 1/32.

On performance of the subpixel function in the wrapper, I removed all std::copy()'s except for one that was needed. This dropped the average time from 8.6s to 5.29s and uses almost no memory. Now, >95% of the time is spent on the loop in process_cmatrix. Can you have a look at process_matrix in openpiv-python-cxx/openpiv_cxx/process/wrapper.cpp to see if there could be any improvements to performance without multi threading?

ErichZimmer commented 2 years ago

I did some profiling on the subpixel wrapper and it appears that nearly all of the time is spent on core::find_peaks. However, it is not too bad as it still beats my naive implementation.

timdewhirst commented 2 years ago

For comparison, and as an indication of how effective optimized builds are:

M1pro, 8 cores, 16GB RAM; 256x256 test images from openpiv-python-cxx, 32x32 with 30x30 overlap/2x2 step giving 12769 vectors; output is from unix command time:

debug build, 7 threads: 10.47s user 0.03s system 665% cpu 1.579 total
debug build, 1 thread: 8.97s user 0.04s system 99% cpu 9.024 total
optimized build, 7 threads: 0.98s user 0.01s system 604% cpu 0.163 total
optimized build, 1 thread: 0.74s user 0.01s system 99% cpu 0.746 total

i.e. 10-12x faster.

Optimized build can be made with cmake -B build -S . -DCMAKE_BUILD_TYPE=Release; cmake --build build on unix, cmake --build build --config Release

timdewhirst commented 2 years ago

@timdewhirst I added some test images that I used for the error testing. Velocity magnitude starts at 0 and maxes out at 2 with a step of 1/32.

On performance of the subpixel function in the wrapper, I removed all std::copy()'s except for one that was needed. This dropped the average time from 8.6s to 5.29s and uses almost no memory. Now, >95% of the time is spent on the loop in process_cmatrix. Can you have a look at process_matrix in openpiv-python-cxx/openpiv_cxx/process/wrapper.cpp to see if there could be any improvements to performance without multi threading?

Sure, will take a look - I think I opined that best performance avoiding copying data and data type conversions was probably going to make a significant difference - your numbers seem to show that! :)

ErichZimmer commented 2 years ago

Setting config to release does indeed create faster code. Time was recorded by jupyter lab's timeit magic function.

process wrapper

debug build, 1 thread: 8.35s ± 139ms
optimized build, 1 thread: 7.97s ± 124ms

subpixel wrapper

debug build, 1 thread: 5.27s ± 32.5ms
optimized build, 1 thread: 2.13s ± 13.3ms

timdewhirst commented 2 years ago

Would it be possible to add some notes on how to build openpiv-python-cxx; I'm trying:

creating a clean venv
from within activated venv: python3 -m pip install OpenPIV-Python-cxx/ which fails with:

% python3 -m pip install OpenPIV-Python-cxx/
Processing ./OpenPIV-Python-cxx
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error

  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [49 lines of output]
      Warning: Subpackage 'openpiv_cxx.preprocess' configuration returned as 'openpiv_cxx.spatial_filters'
      Appending openpiv_cxx.spatial_filters configuration to openpiv_cxx
      Ignoring attempt to set 'name' (from 'openpiv_cxx' to 'openpiv_cxx.spatial_filters')
      non-existing path in 'openpiv_cxx/process': '_libs'
      Appending openpiv_cxx.process configuration to openpiv_cxx
      Ignoring attempt to set 'name' (from 'openpiv_cxx' to 'openpiv_cxx.process')
      Appending openpiv_cxx configuration to
      Ignoring attempt to set 'name' (from '' to 'openpiv_cxx')
      running egg_info
      Traceback (most recent call last):
        File "/Users/tpd/bugless/users/tpd/github/openpiv_python_cxx_venv/lib/python3.9/site-packages/pip/_vendor/pep517/in_process/_in_process.py", line 363, in <module>
          main()
... <snip> ...

ErichZimmer commented 2 years ago

I'll look into when I get home in 8 hours. I threw the setup script together pretty quickly, so there is lots of bugs. In fact, I can identify 3 bugs right now (looking through GitHub)

ErichZimmer commented 2 years ago

@timdewhirst The issue has been fixed (at least on Windows). Additionally, I added some exceptions to help guide the user for proper compilation of the package.

ErichZimmer commented 2 years ago

Did the package install on Unix?

timdewhirst commented 2 years ago

Did the package install on Unix?

No, but I've raised a PR against your repo to make some fixes. It seems to build now but I think there are still issues as when I try to do e.g. import openpiv_cxx.process I get an error:

>>> import openpiv_cxx.process
Traceback (most recent call last):
  File "/Users/tpd/bugless/users/tpd/github/OpenPIV-Python-cxx/openpiv_cxx/process/__init__.py", line 2, in <module>
    from ._libs import _process
ImportError: cannot import name '_process' from 'openpiv_cxx.process._libs' (unknown location)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/tpd/bugless/users/tpd/github/OpenPIV-Python-cxx/openpiv_cxx/process/__init__.py", line 5, in <module>
    raise ValueError("Could not locate either _libs folder or _process wrapper.")
ValueError: Could not locate either _libs folder or _process wrapper.

I suspect this is an environmental issue, but I haven't had the time to go through and figure out the build system yet.

ErichZimmer commented 2 years ago

Have you checked the openpiv_cxx folder in site_packages to see if _libs was copied?

ErichZimmer commented 2 years ago

I got a cmake based build to work using scikit-build that is very flexible. The wrapper is installed by creating a wheel and either conda or pip installing the generated wheel.

OpenPIV / openpiv-c--qt