-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose cpufit API to python #114
Conversation
I also tend to agree that it would make sense to include Cpufit within pyGpufit, because they are coming from the same package. However, there is a (possibly major) caveat that this is counter-intuitive for the user. Someone looking for a curve-fitting algorithm to run on their CPU might skip over a package called "pyGpufit". |
I think that's a fair point. Although I think the main reason one would use Cpufit would be to maintain the same API and build system etc., which currently requires the cuda SDK, so they're probably looking for Gpufit at some point. Cpufit could be copied (or moved) over to a new project? Or maybe the make/cmake config could be changed to allow for an only-Cpufit build, which would not required the cuda SDK? The former is probably too much hassle, and I don't know how to do the latter :) |
If the main use case for Cpufit is for testing or as a fallback option when no GPU is detected, there is no issue with the non-intuitive naming. I would say we go ahead with using the flag |
OK the cmake stuff wasn't so hard, python tests (+1 for cpufit) work for me. P.S. the matlab/etc. bindings will probably be very easy as well from looking at the source, but I can't test those either. |
Ok I got access to a windows machine with a gpu. MSVS is complaining about the new call to
MSVS target 'Cpufit_Gpufit_Test_Consistency' -> Properties -> Linker -> Input -> Additional Dependencies contains Cpufit.lib, so I assume everything is pointing in the correct place. Note that I can build with g++-11 on linux without error. |
@@ -816,11 +816,11 @@ void LMFitCPP::calc_values_gauss2delliptic(std::vector<REAL>& gaussian) | |||
int const size_y = size_x; | |||
for (int iy = 0; iy < size_y; iy++) | |||
{ | |||
REAL const argy = (iy - parameters_[2]) * (iy - parameters_[2]) / (2 * parameters_[4] * parameters_[4]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not necessary, just mirroring gpufit
@@ -1,5 +1,6 @@ | |||
#include "info.h" | |||
#include <algorithm> | |||
#include <limits> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -17,9 +17,9 @@ set( module_files | |||
) | |||
|
|||
if ( USE_CUBLAS AND DEFINED CUBLAS_DLL ) | |||
set( binary $<TARGET_FILE:Gpufit> ${CUBLAS_DLL} ) | |||
set( binary $<TARGET_FILE:Gpufit> ${CUBLAS_DLL} $<TARGET_FILE:Cpufit> ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding cpufit binary dependency to python release
@@ -15,33 +15,43 @@ | |||
package_dir = os.path.dirname(os.path.realpath(__file__)) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate all of the gpufit library wrapping for cpufit
@@ -86,7 +96,7 @@ def _valid_id(cls, id): | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add flag for cpu/gpu
@@ -238,41 +251,45 @@ def fit_constrained(data, weights, model_id, initial_parameters, constraints=Non | |||
chi_squares = np.zeros(number_fits, dtype=np.float32) | |||
number_iterations = np.zeros(number_fits, dtype=np.int32) | |||
|
|||
# argtypes are grabbed from gpufit_func, but are the same for cpufit_func | |||
atypes = gpufit_func.argtypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just cleaning up the name so future devs don't think the argtypes are specific to gpufit
states.ctypes.data_as(gpufit_func.argtypes[15]), \ | ||
chi_squares.ctypes.data_as(gpufit_func.argtypes[16]), \ | ||
number_iterations.ctypes.data_as(gpufit_func.argtypes[17])) | ||
func = {"gpu": gpufit_func, "cpu": cpufit_func}[platform] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call the specific backend depending on the platform flag
assert (chi_squares < 1e-6) | ||
assert (states == 0) | ||
assert (number_iterations <= max_n_iterations) | ||
self.assertTrue(chi_squares < 1e-6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.assertTrue is the py unittest way - it prints the results prettier :)
@@ -26,7 +26,7 @@ def generate_gauss_1d(parameters, x): | |||
|
|||
class Test(unittest.TestCase): | |||
|
|||
def test_gaussian_fit_1d(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gauss_1d shows up in the test from the filename, no need to duplicate
|
||
if __name__ == '__main__': | ||
def test_cpufit(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new cpufit test
clean_resize(cpu.n_iterations, i.n_fits); | ||
|
||
// call to cpufit, store output | ||
int const cpu_status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSVS is not happy about this
@@ -36,7 +36,7 @@ void generate_gauss_2d(std::vector< REAL > & v, std::vector< REAL > const & p) | |||
{ | |||
REAL const argx = ((i - p[1]) * (i - p[1])); | |||
REAL const ex = exp(-(argx + argy) / (2 * p[3] * p[3])); | |||
v[j * n + i] = p[0] * ex + p[3]; | |||
v[j * n + i] = p[0] * ex + p[4]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple bug
REAL tolerance; | ||
int max_n_iterations; | ||
|
||
std::vector< REAL > user_info_; // user info is REAL | ||
|
||
std::vector< REAL> expected_parameters; // size zero to not check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this vector holds the true_parameters we set up with, then we compare fit result with this vector, to test for correctness.
The above were comments to help if anyone reads the PR.
MSVS builds with those functions commented out, but cpufit_constrained cannot be found by python in the cpufit dll. |
Friendly bump, happy for this to not be merged as there's quite a lot of changes. Would appreciate any tips to get it to link properly on windows though, so my lab can use it. Cheers |
Changes: 1. fix bugs in cpufit test functions 2. add test for correctness in Consistency.cpp. 3. add tests for cpufit alone (for when no gpu) to Consistency.cpp 4. add simple tests for constrained fit in Consistency.cpp 5. attempt (incomplete) to add cpufit api to pyGpufit Related to #110 amend cmake to copy cpufit library over to pygpufit clean up python/pyGpufit unit tests clean up of python/pyGpufit unit tests
Alternative approached merged in PR #125 |
Requirements to expose cpufit API to python:
platform='gpu'
by default, which could be changed to cpu fitting viaplatform='cpu'
.Now, assuming we expose cpufit in pyGpufit:
Changes made:
Related to #110