deleting and testing #4092

shubham808 · 2018-01-20T13:16:00Z

Fixes #4074
WIP
Please review this and guide me further.

karlnapf

This is a big and scary PR.
Let's try to make it a bit smaller:

1.) Only the modifications to not use the computation classes anymore, including your openmp changes
2.) Only deleting files

It should be quite straightforward to pull out the changes from here and open another PR with them. Leave this one here open and re-base it later once the other one is merged. I will comment inline on what parts I'd like to review separately

karlnapf · 2018-01-20T15:14:37Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

-	// for storing the aggregators that submit_jobs return
-	CDynamicObjectArray* aggregators=new CDynamicObjectArray();
+	// for storing the result
+	CDynamicArray<float64_t> aggregators;


Either use Std::vector,or (since you know the size in advance) sgvector

karlnapf · 2018-01-20T16:58:26Z

benchmarks/sparse_test.cpp

 *
- * Written (W) 2013 Soumyajit De


I would like to not see these changes in the PR. They are unrelated.

karlnapf

You need to rebase against the latest develop version. There are a lot of files shown here that were changed by someone else

karlnapf · 2018-01-20T17:00:36Z

The rebase needs to happen before I can review anything. This is just too big

karlnapf · 2018-01-20T17:01:33Z

examples/undocumented/python/mathematics_logdet.py

@@ -50,16 +49,13 @@ def mathematics_logdet (matrix=mtx,max_iter_eig=1000,max_iter_lin=1000,num_sampl
 		lin_solver=CGMShiftedFamilySolver()
 		lin_solver.set_iteration_limit(max_iter_lin)

-		# computation engine
-		engine=SerialComputationEngine()


This is an example of a change that should be in the other PR, as it changes usage of the rational approx framework

karlnapf · 2018-01-20T17:02:13Z

src/interfaces/swig/Library.i

-%rename (JobResult) CJobResult;
-%include <shogun/lib/computation/jobresult/JobResult.h>
-%include <shogun/lib/computation/jobresult/ScalarResult.h>
+<<<<<<< HEAD


you have some serious git issues in here ;)
Need to read on how to rebase and how all of git works really. Never commit merge conflicts into a PR

Yeah I did make a lot mistakes sorry about that

No worries at all. Next time, just make sure to have a look at the diff of your PR in github yourself before you press "send".

karlnapf · 2018-01-20T17:02:40Z

src/interfaces/swig/Library.i

-#ifdef USE_INT32
-  %template(ScalarIntResult) CScalarResult<int32_t>;
-#endif
-#ifdef USE_UINT32


this can come in a second patch

karlnapf · 2018-01-20T17:03:43Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

 	index_t num_trace_samples=m_trace_sampler->get_num_samples();
-
-	for (index_t i=0; i<num_estimates; ++i)
+	// for omp


great that you use omp here.
Pull this out into the separate patch then I can review it smoothly

karlnapf · 2018-01-20T17:04:42Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

+
+#ifdef WIN32 // HAVE WIN32
+		for (int32_t vec = start; vec < end; vec++)
+#else


what is the purpose of this contraption?

In case if openmp is not supported this will work like a serial implementation i found a similar thing at shogun shogun/features/DotFeatures.cpp

I think a better would be to look at something like this:
https://github.com/shogun-toolbox/shogun/blob/develop/src/shogun/regression/LeastAngleRegression.cpp#L251

Interesting.... this is easier i will use that for sure.

karlnapf · 2018-01-20T17:05:43Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

+#ifdef HAVE_OPENMP // HAVE OPENMP
+#pragma omp single
+		{
+			num_threads = omp_get_num_threads();


use CParallel

karlnapf · 2018-01-20T17:06:18Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

-			aggregators->append_element(agg);
-			SG_UNREF(agg);
+				// get the trace sampler vector
+				SGVector<float64_t> s = m_trace_sampler->sample(j);


is this method thread-safe (const)?

karlnapf · 2018-01-20T17:06:29Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

+				// get the trace sampler vector
+				SGVector<float64_t> s = m_trace_sampler->sample(j);
+				// calculate the result for sample s
+				float64_t agg = m_operator_log->solve(s);


thread safety

karlnapf · 2018-01-20T17:06:50Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

+				float64_t agg = m_operator_log->solve(s);
+#pragma omp critical // so that the dynamic array stays concurrent
+				{
+					aggregators.append_element(agg);


definitely not thread safe. But if you use a vector as mentioned above, this should be ok

karlnapf · 2018-01-20T17:07:00Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

-	m_computation_engine->wait_for_all();
-	SG_INFO("All jobs finished, aggregating results\n");
+	// wait for all the computations to be completed
+	SG_INFO("Waiting for computations to finish\n");


no need to have that comment

karlnapf · 2018-01-20T17:07:42Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

-	SG_INFO("All jobs finished, aggregating results\n");
+	// wait for all the computations to be completed
+	SG_INFO("Waiting for computations to finish\n");
+#pragma omp barrier


don't think you need barriers. The loop is simple and you can just use #pragma omp parallel

I tried parallel but didnt do the trick maybe i will try again

karlnapf · 2018-01-20T17:08:23Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

-
-		// iterate through indices, group results in the same way as jobs
-		samples[idx_col]+=r->get_result();
+		samples[idx_col] += aggregators.get_element(i);


why don't you remove the aggregators variable and directly store in samples?

i will use samples directly in the next patch

karlnapf · 2018-01-20T17:09:10Z

src/shogun/mathematics/linalg/ratapprox/logdet/LogDetEstimator.cpp

-	// for storing the aggregators that submit_jobs return
-	CDynamicObjectArray aggregators;
-	index_t num_trace_samples=m_trace_sampler->get_num_samples();
+	// for storing the result


there is so much overlapping code here. Could we maybe refactor this to avoid that?
@lambday what are your thoughts here?

karlnapf

Some more initial feedback on the files that matter in the first PR

shubham808 · 2018-01-20T17:16:38Z

@karlnapf i have removed all classes in logdet/computation too because they wont be needed if we use the new solve methods like i have... is that okay ?

karlnapf · 2018-01-20T18:54:35Z

Yes, but pls send that in a sep. PR that includes only deletions

shubham808 · 2018-04-03T14:14:04Z

@karlnapf we should close this one since everything related to it is already addressed elsewhere. :)

shubham808 added 3 commits January 20, 2018 17:17

deleting and testing

9b3294a

Delete and fix conflicts

1acdf7c

fix conflicts

01904ea

karlnapf requested changes Jan 20, 2018

View reviewed changes

karlnapf reviewed Jan 20, 2018

View reviewed changes

benchmarks/sparse_test.cpp

*

* Written (W) 2013 Soumyajit De

Copy link

Member

karlnapf Jan 20, 2018

Choose a reason for hiding this comment

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

I would like to not see these changes in the PR. They are unrelated.

karlnapf requested changes Jan 20, 2018

View reviewed changes

karlnapf reviewed Jan 20, 2018

View reviewed changes

karlnapf requested changes Jan 20, 2018

View reviewed changes

shubham808 mentioned this pull request Jan 22, 2018

Refactoring #4103

Merged

shubham808 closed this Apr 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deleting and testing #4092

deleting and testing #4092

shubham808 commented Jan 20, 2018

karlnapf left a comment

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf left a comment

karlnapf commented Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

shubham808 Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018 •

edited

karlnapf Jan 20, 2018

shubham808 Jan 20, 2018

karlnapf Jan 20, 2018

shubham808 Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf Jan 20, 2018

shubham808 Jan 20, 2018 •

edited

karlnapf Jan 20, 2018

shubham808 Jan 20, 2018

karlnapf Jan 20, 2018

karlnapf left a comment

shubham808 commented Jan 20, 2018 •

edited

karlnapf commented Jan 20, 2018

shubham808 commented Apr 3, 2018

deleting and testing #4092

deleting and testing #4092

Conversation

shubham808 commented Jan 20, 2018

karlnapf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

karlnapf commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf Jan 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham808 Jan 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

shubham808 commented Jan 20, 2018 • edited

karlnapf commented Jan 20, 2018

shubham808 commented Apr 3, 2018

karlnapf Jan 20, 2018 •

edited

shubham808 Jan 20, 2018 •

edited

shubham808 commented Jan 20, 2018 •

edited