[GitHub] madlib pull request #251: MLP: Simplify initialization of model coefficients

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/madlib/pull/251


---

[GitHub] madlib pull request #251: MLP: Simplify initialization of model coefficients

[iyerr3]

Github user iyerr3 commented on a diff in the pull request:

https://github.com/apache/madlib/pull/251#discussion_r178204767
  
--- Diff: src/modules/convex/mlp_igd.cpp ---
@@ -98,23 +98,28 @@ mlp_igd_transition::run(AnyType ) {
 
 double is_classification_double = (double) is_classification;
 double activation_double = (double) activation;
-MappedColumnVector coeff = 
args[10].getAs();
-
state.task.model.rebind(_classification_double,_double,
-()[0], numberOfStages,
-[0]);
-
-// state.task.model.is_classification =
-// static_cast(is_classification);
-// state.task.model.activation = 
static_cast(activation);
-// MappedColumnVector initial_coeff = 
args[10].getAs();
-// // copy initial_coeff into the model
-// Index fan_in, fan_out, layer_start = 0;
-// for (size_t k = 0; k < numberOfStages; ++k){
-// fan_in = numbersOfUnits[k];
-// fan_out = numbersOfUnits[k+1];
-// state.task.model.u[k] << 
initial_coeff.segment(layer_start, (fan_in+1)*fan_out);
-// layer_start = (fan_in + 1) * fan_out;
-// }
+if (!args[10].isNull()){
+// initial coefficients are provided
+MappedColumnVector warm_start_coeff = 
args[10].getAs();
+
+// copy warm start into the task model
+// state.reset() ensures algo.incrModel is copied from 
task.model
+Index layer_start = 0;
+for (size_t k = 0; k < numberOfStages; ++k){
+for (size_t j=0; j < state.task.model.u[k].cols(); 
++j){
+for (size_t i=0; i < state.task.model.u[k].rows(); 
++i){
+state.task.model.u[k](i, j) = warm_start_coeff(
+layer_start + j * 
state.task.model.u[k].rows() + i);
+}
+}
+layer_start = state.task.model.u[k].rows() * 
state.task.model.u[k].cols();
+}
+} else {
+// initialize the model with appropriate coefficients
+state.task.model.initialize(
--- End diff --

The comment in line 106 is actually for both the warm start copy and the 
initialize. Basically `state.reset()` in line 125 is copying `task.model` to 
`algo.incrModel`. 


---

[GitHub] madlib pull request #251: MLP: Simplify initialization of model coefficients

[njayaram2]

Github user njayaram2 commented on a diff in the pull request:

https://github.com/apache/madlib/pull/251#discussion_r178204067
  
--- Diff: src/modules/convex/mlp_igd.cpp ---
@@ -98,23 +98,28 @@ mlp_igd_transition::run(AnyType ) {
 
 double is_classification_double = (double) is_classification;
 double activation_double = (double) activation;
-MappedColumnVector coeff = 
args[10].getAs();
-
state.task.model.rebind(_classification_double,_double,
-()[0], numberOfStages,
-[0]);
-
-// state.task.model.is_classification =
-// static_cast(is_classification);
-// state.task.model.activation = 
static_cast(activation);
-// MappedColumnVector initial_coeff = 
args[10].getAs();
-// // copy initial_coeff into the model
-// Index fan_in, fan_out, layer_start = 0;
-// for (size_t k = 0; k < numberOfStages; ++k){
-// fan_in = numbersOfUnits[k];
-// fan_out = numbersOfUnits[k+1];
-// state.task.model.u[k] << 
initial_coeff.segment(layer_start, (fan_in+1)*fan_out);
-// layer_start = (fan_in + 1) * fan_out;
-// }
+if (!args[10].isNull()){
+// initial coefficients are provided
+MappedColumnVector warm_start_coeff = 
args[10].getAs();
+
+// copy warm start into the task model
+// state.reset() ensures algo.incrModel is copied from 
task.model
+Index layer_start = 0;
+for (size_t k = 0; k < numberOfStages; ++k){
+for (size_t j=0; j < state.task.model.u[k].cols(); 
++j){
+for (size_t i=0; i < state.task.model.u[k].rows(); 
++i){
+state.task.model.u[k](i, j) = warm_start_coeff(
+layer_start + j * 
state.task.model.u[k].rows() + i);
+}
+}
+layer_start = state.task.model.u[k].rows() * 
state.task.model.u[k].cols();
+}
+} else {
+// initialize the model with appropriate coefficients
+state.task.model.initialize(
--- End diff --

This is more of a doubt than a comment I guess. Should this be 
`state.algo.incrModel` instead?
The reason I thought so was that `MLPIGDAlgorithm::transition(state, 
tuple);` uses the model in algo instead of the one task.


---

[GitHub] madlib pull request #251: MLP: Simplify initialization of model coefficients

[iyerr3]

GitHub user iyerr3 opened a pull request:

https://github.com/apache/madlib/pull/251

MLP: Simplify initialization of model coefficients

Changes:

1. Model initialization now happens in the C++ code instead of being
passed via Python.
2. Warm start coefficients are still passed from Python. These
coefficients are copied into the model. The vectorized form of the copy
raises an Eigen assertion when used with a MappedMatrix, possibly due
to the actual block size not being available via the Map.
3. The distinction between a "TaskState" and an "AlgoState" within a
State type has been removed in the MLPMiniBatchState. This demarcation
seemed to be confusing and didn't provide much abstraction.

Closes #251

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/iyerr3/incubator-madlib 
bugfix/minibatch_init_debug

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/madlib/pull/251.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #251






---