Refactoring #4103

shubham808 · 2018-01-22T20:20:28Z

#4074 #4092
[WIP]
This is part 1 here we will refactor code to change the use of the computation classes to be deleted.
Part 2 will including deleting the classes.
I am struggling with RationalApproximation unit test.
Please review this and guide me further.

vigsterkr

thxn for the patch!
here are some comments/requests thnx

vigsterkr · 2018-01-23T07:13:00Z

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

+#pragma omp critical // so that the dynamic array stays concurrent
+				{
+					samples[idx_col] += result;
+					idx_row++;


statements like these use preoperator and not the post operator

I have directly used the iterator i here which simplifies it.

vigsterkr · 2018-01-23T07:15:27Z

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

+				float64_t result = m_operator_log->solve(s);
+#pragma omp critical // so that the dynamic array stays concurrent
+				{
+					samples[idx_col] += result;


this could be reformulated instead of using critical using parallel for reduction...
check for example:
http://cs.umw.edu/~finlayson/class/fall16/cpsc425/notes/11-parfor.html

Looks useful I will try to implement it.

vigsterkr · 2018-01-23T07:18:29Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationCGM.cpp

+	SGVector<float64_t> agg = m_linear_operator->apply(vec.get_imag());
+
+	// perform dot product
+	Map<VectorXd> map_agg(agg.vector, agg.vlen);


yeah no... i mean i was already about to comment above that why do we use eigen here directly... but now seeing that it's being used to do the dot product below.... plz use linalg:: over SGVector/Matrix

I have used linalg in all opfunc classes however the DenseMatrixExactJob class use mat.log() which is not in linalg so it still uses Eigen3

vigsterkr · 2018-01-23T07:19:14Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationIndividual.cpp

-		m_computation_engine->submit_job(job);
+		// multiply with the weight using Eigen3 and take negative
+		// (see CRationalApproximation for the formula)
+		Map<VectorXcd> v(vec.vector, vec.vlen);


use linalg:: for linear algebra operations

karlnapf · 2018-01-23T09:54:19Z

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

+				SGVector<float64_t> s = m_trace_sampler->sample(j);
+				// calculate the result for sample s
+				float64_t result = m_operator_log->solve(s);
+#pragma omp critical // so that the dynamic array stays concurrent


which dynamic array?

typo error because we decided to not use any kind of aggregators and directly use samples

karlnapf · 2018-01-23T09:54:39Z

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

@@ -223,67 +170,41 @@ SGMatrix<float64_t> CLogDetEstimator::sample_without_averaging(
 	SG_DEBUG("Entering...\n")

 	REQUIRE(m_operator_log, "Operator function is NULL\n");
-	// call the precompute of operator function to compute all prerequisites
+	// call the precompute of operator function zto compute all prerequisites


karlnapf · 2018-01-23T09:54:58Z

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

+				SGVector<float64_t> s = m_trace_sampler->sample(j);
+				// solve the result for s
+				float64_t result = m_operator_log->solve(s);
+#pragma omp critical // aggregators array should be concurrent


use reduce here as well

karlnapf · 2018-01-23T09:55:30Z

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

- * to ensure that the job result aggregators are all up to date. Then simply
- * computes running averages over the estimates
+ * and calls solve of COperatorFunction, stores the resulting in
+ * a vector, Then simply computes running averages over the estimates


type "Then" should be "then"

karlnapf · 2018-01-23T09:56:27Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/DenseMatrixExactLog.cpp

-	: COperatorFunction<float64_t>(NULL, NULL, OF_LOG)
-{
-	SG_GCDEBUG("%s created (%p)\n", this->get_name(), this)
+	CDenseMatrixExactLog::CDenseMatrixExactLog()


you have a lot of whitespace changes not sure whether that is due to the auto-formatter.
In any case, search for "style" in the first travis job output to get the comment how to fix formatting

This was a style issue I have fixed it now... i think

karlnapf · 2018-01-23T09:57:08Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/DenseMatrixExactLog.cpp

+
+	SGVector<float64_t> vec = m_log_operator->apply(sample);
+
+	// compute the vector-vector dot product using Eigen3


remove this comment, doesnt add any valu
also, you can just use linalg::dot

karlnapf · 2018-01-23T09:58:32Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationCGM.h

 	 */
-	virtual CJobResultAggregator* submit_jobs(SGVector<float64_t> sample);
+	virtual float64_t solve(SGVector<float64_t> sample);


compute?

Also, make sure to start the comment with a capital M. "Method to ..."

karlnapf · 2018-01-23T09:59:25Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationIndividual.cpp

@@ -107,7 +97,7 @@ CJobResultAggregator* CLogRationalApproximationIndividual::submit_jobs(
 	else
 	{
 		// something weird happened
-		SG_ERROR("OperatorFunction::submit_jobs(): Unknown MatrixOperator given!\n");
+		SG_ERROR("OperatorFunction::solve(): Unknown MatrixOperator given!\n");


in all error messages that you touch, remove the ClassName::method_name, this is outdated

karlnapf · 2018-01-23T10:00:50Z

src/shogun/mathematics/linalg/ratapprox/opfunc/OperatorFunction.h

-	 * abstract precompute method that must be called before using submit jobs
-	 * for performing preliminary computations that are necessary for the
-	 * rest of the computation jobs
+	 * abstract precompute method that must be called before using solve


"Purely virtual method that ..."

karlnapf · 2018-01-23T10:01:04Z

src/shogun/mathematics/linalg/ratapprox/opfunc/OperatorFunction.h

-	 *
-	 * @param sample the vector for which new computation job(s) are to be created
-	 * @return the array of generated independent jobs
+	 * method that solves for a sample and returns the final result


"Method that solves ..."

or better "computes" and call it compute

karlnapf · 2018-01-23T10:02:13Z

src/shogun/mathematics/linalg/ratapprox/opfunc/RationalApproximation.h

-	 *
-	 * @param sample the vector for which new computation job(s) are to be created
-	 * @return the array of generated independent jobs
+	 *method that solves for a particular sample


"* Method that computes ...."

karlnapf · 2018-01-23T10:02:45Z

tests/unit/mathematics/linalg/DenseMatrixExactLog_unittest.cc

 	SG_REF(op_func);

 	// its really important we call the precompute on the operato function
 	op_func->precompute();

-	// for storing the aggregators that submit_jobs return
-	CDynamicObjectArray aggregators;
+	float64_t result = 0.0;

 	// create samples for extracting the trace of log(C) and submit
 	for (index_t i=0; i<size; ++i)


maybe this can be parallell as well?
or is it inside the other parallel loop?

this is the other loop and i think we can keep it as is maybe ?

karlnapf · 2018-01-23T10:03:31Z

tests/unit/mathematics/linalg/RationalApproximation_unittest.cc

-	// create the aggregators to contain the result aggregators
-	CDynamicObjectArray aggregators;
-
+	float64_t result = 0.0;
 	// extract the trace of approximation of log using basis vectors
 	for (index_t i=0; i<size; ++i)


maybe parallel as well?
Generally, I would put the omp into the outmost loop

karlnapf

This is a great patch. Thanks!

And also much easier to review in detail, which we now did. I think the changes should be quick to implement.

Looking forward to merging it soon!

karlnapf · 2018-01-23T10:04:45Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationIndividual.cpp

 #include <shogun/lib/SGMatrix.h>
-#include <shogun/mathematics/linalg/linsolver/LinearSolver.h>
+#include <shogun/lib/SGVector.h>
+#include <shogun/mathematics/eigen3.h>


I think the eigen3 includes can go once you use linalg

karlnapf · 2018-01-23T10:05:04Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationIndividual.cpp

-{
-	init();
+	CLogRationalApproximationIndividual::CLogRationalApproximationIndividual()
+	    : CRationalApproximation(NULL, NULL, 0, OF_LOG)


could you use nullptr here?

karlnapf · 2018-01-23T10:05:21Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/LogRationalApproximationIndividual.cpp


-	SG_GCDEBUG("%s created (%p)\n", this->get_name(), this)
+		SG_GCDEBUG("%s created (%p)\n", this->get_name(), this)


Delete this, not needed

I deleted it from all opfunc classes because it appears to be deprecated. is it okay ?

shubham808 · 2018-01-23T19:50:08Z

To be Done ->
using reduction in openmp loops. I will try to do this soon.
Fixing RationalApproximation unit test fail. I am not able to figure out why but i will keep trying.

karlnapf · 2018-01-24T15:12:32Z

I just had a thought: we can also separate the openmp stuff into a new PR
This way, this one would just be refactoring without using the computation stuff, and then next one would be deleting the computation folder, finally, we would work on openmp separately from the refactoring (might be easier)
I will leave that to you

karlnapf · 2018-01-24T15:14:00Z

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

-		samples[idx_col]+=r->get_result();
-		idx_row++;
-		if (idx_row>=num_trace_samples)
+	// for omp


don't put those comments in, they don't add information :)

(as I said, we can postpone the openmp into a new PR to make things easier to handle if you wanted to)

karlnapf · 2018-01-24T17:01:53Z

What fails in the unit test?

shubham808 · 2018-01-24T17:05:20Z

The RationalApproximation.trace_accuracy fails missing the correct value by a large amount. ( expected is -11.5 but we are getting -4.1 ) and the expected error is 1E-07

shubham808 · 2018-01-24T17:06:10Z

Lets do the openmp stuff later because i am not sure how to deal with reduction of arrays... we can include that in a seperate pr.

karlnapf · 2018-01-24T17:24:55Z

Ok so why don't you remove all the omp calls then for now.

karlnapf · 2018-01-24T17:27:04Z

tests/unit/mathematics/linalg/RationalApproximation_unittest.cc

-		// its important that we don't just unref the result here
-		result+=r->get_result();
-		SG_UNREF(agg);
+		result += op_func->compute(s);


it seems you are not changing anything here ... don't understand why the test fails.

yeah i am confused too... also the RationalApproximationCGM tests pass its the one with
RationalApproximationIndividual that fail.

you will have to carefully backtrace the changes.
If staring at the code doesnt help, you can run the two implementations next to each other in a debugger and ensure that the same number is added in each step. Ping me if you need help with this, I am in IRC for another hour

okay i am on it.

shubham808 · 2018-02-04T11:32:00Z

@karlnapf sorry for the late reply.... i am still stuck at the tests. in @lambday implementation only this test uses a separate aggregator and i am still not sure of its purpose

karlnapf · 2018-02-04T12:27:42Z

We now have a merge conflict as well.

@lambday can you help with the aggregator?

vigsterkr · 2018-02-06T08:29:50Z

@shubham808 how is it going with the PR? could you rebase it over the latest develop?

vigsterkr · 2018-02-06T08:37:39Z

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

-		idx_row++;
-		if (idx_row>=num_trace_samples)
+
+#pragma omp parallel for


#pragma omp parallel for reduction(+: samples[:num_estimates])

this should work with openmp 4.5

yeah this looks good.

dont forget to remove the critical part

shubham808 · 2018-02-06T09:05:38Z

This pr is still WIP because of unit test fails.
I will rebase and make a commit soon :)

karlnapf · 2018-02-06T10:50:01Z

@vigsterkr we had also split things a bit to make it easier to oversee
first patch is to refactor existing code/tests to not use the computation framework.
next is deleting the now unsused code
next is using omp

vigsterkr · 2018-02-06T10:50:58Z

@karlnapf whatever is fine... just not the current state of omp

vigsterkr · 2018-03-22T09:23:43Z

this needs rebase :S

shubham808 · 2018-03-23T08:28:04Z

@vigsterkr I removed omp for now along with rebase and minor edits ...

vigsterkr · 2018-03-23T08:44:51Z

src/shogun/machine/Machine.h

@@ -455,6 +455,5 @@ class CMachine : public CSGObject
 		/** Mutex used to pause threads */
 		std::mutex m_mutex;
 };
-


please avoid these type of hanges

must have been the style formatting

vigsterkr · 2018-03-23T08:45:01Z

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

@@ -4,7 +4,6 @@
 * Authors: Sunil Mahendrakar, Heiko Strathmann, Soumyajit De, Björn Esser
 */
 #include <shogun/base/Parallel.h>
-#include <shogun/base/progress.h>


It was not being used anywhere... we could put it back in when we add progress bar to it (after this gets merged )

vigsterkr · 2018-03-23T08:45:43Z

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

-	// for omp
-#pragma omp parallel for
-		for (index_t i = 0; i < num_estimates; i++)
+	// TO DO: use openmp like this here->#pragma omp parallel for reduction(+:


why is it a todo if it is the solution? if not then just add the todo but not the whole pragma.. have you tested the pragma itself?

the pragma didnt work.... error: 'samples' does not have pointer or array type

vigsterkr · 2018-03-23T08:46:22Z

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

-		for (index_t i = 0; i < num_estimates; i++)
+	// TO DO: use openmp like this here->#pragma omp parallel for reduction(+:
+	// samples[:num_estimates])
+	for (index_t i = 0; i < num_estimates; i++)


why post operator here (i++) but prefix-operator below (++j). i'd stick with prefix

vigsterkr · 2018-03-23T08:47:00Z

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

-		for (index_t i = 0; i < num_estimates; i++)
+	// TO DO: use openmp #pragma omp parallel for reduction(+:
+	// samples[:num_estimates][:num_trace_samples])
+	for (index_t i = 0; i < num_estimates; i++)


same story... prefix vs post.
btw this code seems to be a bit of a copy-paste from the one above

vigsterkr · 2018-03-23T08:47:38Z

src/shogun/mathematics/linalg/ratapprox/logdet/opfunc/DenseMatrixExactLog.cpp

+	CDenseMatrixExactLog::CDenseMatrixExactLog(
+	    CDenseMatrixOperator<float64_t>* op)
+	    : COperatorFunction<float64_t>((CLinearOperator<float64_t>*)op, OF_LOG)
+	{


why the removal of the DEBUG-ing message?

That change was requested because it was not very informative

shubham808 · 2018-04-01T12:06:57Z

🎉

shubham808 · 2018-04-02T11:03:19Z

@vigsterkr @karlnapf any changes ?

karlnapf · 2018-04-02T14:34:51Z

I think we are good. Thanks for this monster effort!
Next PR would be to continue with the other changes we discussed: removing all the classes that are now not used anymore, paralleling the current code using openmp.

* Refactoring * Update LogDetEstimator.cpp * rebase and edits * fixes RationalApproximation unit test * style fixes

vigsterkr requested changes Jan 23, 2018

View reviewed changes

karlnapf reviewed Jan 23, 2018

View reviewed changes

karlnapf requested changes Jan 23, 2018

View reviewed changes

karlnapf reviewed Jan 23, 2018

View reviewed changes

shubham808 force-pushed the feature/refactoring_logdet_1 branch from bcc6630 to fda4791 Compare January 23, 2018 19:47

karlnapf reviewed Jan 24, 2018

View reviewed changes

vigsterkr requested changes Feb 6, 2018

View reviewed changes

shubham808 force-pushed the feature/refactoring_logdet_1 branch 2 times, most recently from a352bdc to 271938f Compare March 23, 2018 04:00

vigsterkr requested changes Mar 23, 2018

View reviewed changes

shubham808 added 4 commits April 1, 2018 15:15

Refactoring

899623c

Update LogDetEstimator.cpp

9ee2046

rebase and edits

373bc96

fixes RationalApproximation unit test

24ee5dc

shubham808 force-pushed the feature/refactoring_logdet_1 branch from 8c4bd86 to 25cf5a6 Compare April 1, 2018 11:19

style fixes

601eb39

shubham808 force-pushed the feature/refactoring_logdet_1 branch from 25cf5a6 to 601eb39 Compare April 1, 2018 13:23

karlnapf approved these changes Apr 2, 2018

View reviewed changes

karlnapf merged commit 8d578f4 into shogun-toolbox:develop Apr 2, 2018

karlnapf mentioned this pull request Apr 4, 2018

Refactor log-determinant estimation code to work without computation engines #4074

Closed

shubham808 mentioned this pull request Apr 6, 2018

parrallel computation of sample in LogDetEstimator #4235

Merged

ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019

Refactoring (shogun-toolbox#4103)

c72abbe

* Refactoring * Update LogDetEstimator.cpp * rebase and edits * fixes RationalApproximation unit test * style fixes


		SGVector<float64_t> vec = m_log_operator->apply(sample);

		// compute the vector-vector dot product using Eigen3


		SG_GCDEBUG("%s created (%p)\n", this->get_name(), this)
		SG_GCDEBUG("%s created (%p)\n", this->get_name(), this)

Refactoring #4103

Refactoring #4103

Conversation

shubham808 commented Jan 22, 2018

vigsterkr left a 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

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

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

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 commented Jan 23, 2018 • edited

karlnapf commented Jan 24, 2018

Choose a reason for hiding this comment

karlnapf commented Jan 24, 2018

shubham808 commented Jan 24, 2018 • edited

shubham808 commented Jan 24, 2018

karlnapf commented Jan 24, 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

shubham808 commented Feb 4, 2018

karlnapf commented Feb 4, 2018

vigsterkr commented Feb 6, 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

shubham808 commented Feb 6, 2018

karlnapf commented Feb 6, 2018

vigsterkr commented Feb 6, 2018

vigsterkr commented Mar 22, 2018

shubham808 commented Mar 23, 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

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 commented Apr 1, 2018

shubham808 commented Apr 2, 2018 • edited

karlnapf commented Apr 2, 2018

shubham808 commented Jan 23, 2018 •

edited

shubham808 commented Jan 24, 2018 •

edited

shubham808 commented Apr 2, 2018 •

edited