[GitHub] madlib pull request #237: Bugfix: MLP predict using 1.12 model fails on late...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] madlib pull request #237: Bugfix: MLP predict using 1.12 model fails on late...

[iyerr3]

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

https://github.com/apache/madlib/pull/237#discussion_r169845958
  
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -749,8 +749,18 @@ def mlp_predict(schema_madlib,
 summary['layer_sizes'], array_type="DOUBLE PRECISION")
 is_classification = int(summary["is_classification"])
 is_response = int(pred_type == 'response')
-grouping_col = '' if summary['grouping_col']=='NULL' \
-else summary['grouping_col']
+if 'grouping_col' in summary:
+# This model was created in MADlib 1.13 or greater version
+is_v112_model = False
--- End diff --

I prefer calling the flag `is_pre_113_model` as that makes the intention 
clearer. 


---

[GitHub] madlib pull request #237: Bugfix: MLP predict using 1.12 model fails on late...

[iyerr3]

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

https://github.com/apache/madlib/pull/237#discussion_r169846145
  
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -796,14 +807,34 @@ def mlp_predict(schema_madlib,
 else:
 # if not grouping, then directly read out the coeff, mean
 # and std values from the model and standardization tables.
-standardization = plpy.execute(
-"SELECT * FROM {0}".format(standardization_table))[0]
+
+# Fix to ensure that 1.12 models run on 1.13 or higher.
+# As a result of adding grouping support in 1.13, the following 
change
+# was also made wrt standardization. The x_mean and x_std
+# values were stored in the summary table itself in MADlib 1.12, 
and
+# they were named as: x_means and x_stds.
+# From MADlib 1.13 onwards, these parameters were moved to the
+# _standardization table, and were renamed to mean and std.
+if is_v112_model:
--- End diff --

I feel we can simplify by creating two SQL strings and avoid all the 
variables:  
```
if pre_113:  
SELECT x_means as mean, x_stds as std FROM {summary_table}
else: 
SELECT mean, std FROM {standardization_table}
```



---

[GitHub] madlib pull request #237: Bugfix: MLP predict using 1.12 model fails on late...

[iyerr3]

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

https://github.com/apache/madlib/pull/237#discussion_r169846321
  
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -796,14 +807,34 @@ def mlp_predict(schema_madlib,
 else:
 # if not grouping, then directly read out the coeff, mean
 # and std values from the model and standardization tables.
-standardization = plpy.execute(
-"SELECT * FROM {0}".format(standardization_table))[0]
+
+# Fix to ensure that 1.12 models run on 1.13 or higher.
--- End diff --

Maybe move the explanation closer to the creation of the flag. 


---

[GitHub] madlib pull request #237: Bugfix: MLP predict using 1.12 model fails on late...

[njayaram2]

GitHub user njayaram2 opened a pull request:

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

Bugfix: MLP predict using 1.12 model fails on later versions

JIRA: MADLIB-1207

MADlib 1.12 did not support grouping in MLP. The summary table created
used to have the mean and std used for standardizing the independent
variable. From MADlib 1.13 onwards, grouping was supported, and the mean
and std were moved to the standardization table.
This resulted in a failure when MADlib 1.12 MLP models were used to
predict using MADlib 1.13. This commit fixes this issue.

Closes #237

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

$ git pull https://github.com/njayaram2/madlib bug/mlp-1.12model-grouping

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

https://github.com/apache/madlib/pull/237.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 #237


commit 63daad10bf8c610d60be738f79e9bdc22ddc6483
Author: Nandish Jayaram 
Date:   2018-02-22T01:18:57Z

Bugfix: MLP predict using 1.12 model fails on later versions

JIRA: MADLIB-1207

MADlib 1.12 did not support grouping in MLP. The summary table created
used to have the mean and std used for standardizing the independent
variable. From MADlib 1.13 onwards, grouping was supported, and the mean
and std were moved to the standardization table.
This resulted in a failure when MADlib 1.12 MLP models were used to
predict using MADlib 1.13. This commit fixes this issue.

Closes #237




---