[GitHub] madlib pull request #238: MLP: Use array_upper to get the last array element

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] madlib issue #237: Bugfix: MLP predict using 1.12 model fails on later versi...

[asfgit]

Github user asfgit commented on the issue:

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

Refer to this link for build results (access rights to CI server needed): 
https://builds.apache.org/job/madlib-pr-build/358/



---

[GitHub] madlib issue #237: Bugfix: MLP predict using 1.12 model fails on later versi...

[njayaram2]

Github user njayaram2 commented on the issue:

https://github.com/apache/madlib/pull/237
  
Thank you for the comments @iyerr3 , will push another commit.


---

[GitHub] madlib issue #238: MLP: Use array_upper to get the last array element

[fmcquillan99]

Github user fmcquillan99 commented on the issue:

https://github.com/apache/madlib/pull/238
  
LGTM


---

[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. 


---