Skip to content
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

Remove pthread header from SVMLightOneClass #3650

Merged
merged 1 commit into from Feb 28, 2017

Conversation

abhinavrai44
Copy link
Contributor

No description provided.

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some comments by @vigsterkr and splitting into 3 PRs.
Unit tests should be there for single a multi-threaded versions

@@ -33,10 +33,6 @@ extern "C" {

#include <shogun/base/Parallel.h>

#ifdef HAVE_PTHREAD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a sep. PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

int32_t num_threads=1;
#endif
ASSERT(num_threads>0)
int32_t num_threads;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a variable for the number of threads?

params[t].idx_comp=idx_b;

pthread_create(&threads[t], &attr, CDistanceMachine::run_distance_thread_lhs, (void*)&params[t]);
num_threads=omp_get_num_threads();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vigsterkr can you comment on the use of Parallel here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karlnapf The variable involved need to be computed only once as they will remain constant over all the threads. In parallel the same computations will be repeated over all threads.

SG_FREE(params);
SG_FREE(threads);
for(int32_t i=idx_r_start; idx_start < idx_stop; i++,idx_start++)
result[i] = distance->distance(idx_start,idx_b);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether the distance matrix computation should be part of CDistance and then we just call that method from here?

@@ -39,18 +39,11 @@
#include <shogun/mathematics/Math.h>
#include <shogun/lib/Lock.h>

using namespace shogun;
#ifdef HAVE_OPENMP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please do every class in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok :-)

@abhinavrai44 abhinavrai44 force-pushed the feature/openmp branch 2 times, most recently from 072b764 to 3886356 Compare February 27, 2017 19:36
@abhinavrai44 abhinavrai44 changed the title Port DistanceMachine and Inference to use OpenMP Remove pthread header from SVMLightOneClass Feb 27, 2017
@vigsterkr
Copy link
Member

dunno what's happening with travis but i think this one is really safe to merge....

@vigsterkr vigsterkr merged commit 7ec1bf1 into shogun-toolbox:develop Feb 28, 2017
@abhinavrai44 abhinavrai44 deleted the feature/openmp branch March 14, 2017 16:09
karasikov pushed a commit to karasikov/shogun that referenced this pull request Apr 15, 2017
Remove pthread header from SVMLightOneClass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants