You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@madlib.apache.org by njayaram2 <gi...@git.apache.org> on 2018/03/26 19:13:49 UTC
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
GitHub user njayaram2 opened a pull request:
https://github.com/apache/madlib/pull/250
MLP: Allow one-hot encoded dependent var for classification
JIRA:MADLIB-1222
MLP currently automatically encodes categorical variables for
classification but does not allow already encoded arrays for dependent
variables in mlp_classification. This commit lets users have an already
encoded array for the dependent variable and train a model.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/madlib/madlib feature/mlp/support-encoded-dep-var
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/madlib/pull/250.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 #250
----
commit f5a87dee6bc8f27c1a13a4921ea726b391b1813d
Author: Nandish Jayaram <nj...@...>
Date: 2018-03-20T22:43:25Z
MLP: Allow one-hot encoded dependent var for classification
JIRA:MADLIB-1222
MLP currently automatically encodes categorical variables for
classification but does not allow already encoded arrays for dependent
variables in mlp_classification. This commit lets users have an already
encoded array for the dependent variable and train a model.
----
---
[GitHub] madlib issue #250: MLP: Allow one-hot encoded dependent var for classificati...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/250
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/422/
---
[GitHub] madlib issue #250: MLP: Allow one-hot encoded dependent var for classificati...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/250
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/408/
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by njayaram2 <gi...@git.apache.org>.
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r178168263
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
# array for dep type in IGD
- _assert("[]" not in expr_type and expr_type in classification_types,
+ _assert(("[]" in expr_type and expr_type[:-2] in int_types) \
+ or expr_type in classification_types,
"Dependent variable column should be of type: "
--- End diff --
Will change it to cover both the cases.
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r177928648
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
# array for dep type in IGD
- _assert("[]" not in expr_type and expr_type in classification_types,
+ _assert(("[]" in expr_type and expr_type[:-2] in int_types) \
--- End diff --
This check does not differentiate between a 1d array and a 2d array. If you pass a 2d array as the dep variable, both train and predict functions go through and here is what the output of predict looks like
```sql
madlib=# select * from mlp_pred_dep;
id | estimated_prob
-----+----------------
2 | {1}
13 | {1}
15 | {1}
28 | {1}
33 | {1}
```
We don't have to necessarily worry about this, we can just add a note in the user docs but I just wanted to mention it. What do you think ?
---
[GitHub] madlib issue #250: MLP: Allow one-hot encoded dependent var for classificati...
Posted by fmcquillan99 <gi...@git.apache.org>.
Github user fmcquillan99 commented on the issue:
https://github.com/apache/madlib/pull/250
See JIRA https://issues.apache.org/jira/browse/MADLIB-1222 for examples showing this works for IGD and mini-batch
LGTM
I think u can go ahead and merge this PR to master
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r177912480
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
# array for dep type in IGD
- _assert("[]" not in expr_type and expr_type in classification_types,
+ _assert(("[]" in expr_type and expr_type[:-2] in int_types) \
--- End diff --
why are we checking for only int types and not all numeric types ?
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/madlib/pull/250
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by njayaram2 <gi...@git.apache.org>.
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r178167691
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
--- End diff --
Yes, that's correct.
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by njayaram2 <gi...@git.apache.org>.
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r178168389
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -856,8 +868,16 @@ def mlp_predict(schema_madlib, model_table, data_table, id_col_name,
activation = _get_activation_index(summary['activation'])
layer_sizes = PY2SQL(
summary['layer_sizes'], array_type="DOUBLE PRECISION")
- is_classification = int(summary["is_classification"])
is_response = int(pred_type == 'response')
+ is_classification = int(summary["is_classification"])
+ classes = summary['classes']
+ # Set a flag to indicate that it is a classification model, with an array
+ # as the dependent var. The only scenario where classification allows for
+ # an array dep var is when the user has provided a one-hot encoded dep var
+ # during training, and mlp_classification does not one-hot encode
+ # (and hence classes column in model's summary table is NULL).
+ array_dep_var_for_classification = int(is_classification and not classes)
--- End diff --
Will change it to `is_dep_var_an_array_for_classification`
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r177913869
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -856,8 +868,16 @@ def mlp_predict(schema_madlib, model_table, data_table, id_col_name,
activation = _get_activation_index(summary['activation'])
layer_sizes = PY2SQL(
summary['layer_sizes'], array_type="DOUBLE PRECISION")
- is_classification = int(summary["is_classification"])
is_response = int(pred_type == 'response')
+ is_classification = int(summary["is_classification"])
+ classes = summary['classes']
+ # Set a flag to indicate that it is a classification model, with an array
+ # as the dependent var. The only scenario where classification allows for
+ # an array dep var is when the user has provided a one-hot encoded dep var
+ # during training, and mlp_classification does not one-hot encode
+ # (and hence classes column in model's summary table is NULL).
+ array_dep_var_for_classification = int(is_classification and not classes)
--- End diff --
can we change the name of `array_dep_var_for_classification` to something like`is_dep_var_an_array` so that it's clear that it's a flag.
---
[GitHub] madlib issue #250: MLP: Allow one-hot encoded dependent var for classificati...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/250
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/416/
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r177912997
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
--- End diff --
This comment is not relevant anymore.
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r177912305
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
# array for dep type in IGD
- _assert("[]" not in expr_type and expr_type in classification_types,
+ _assert(("[]" in expr_type and expr_type[:-2] in int_types) \
+ or expr_type in classification_types,
"Dependent variable column should be of type: "
--- End diff --
This error message is not reflective of the error check , right ?
---
[GitHub] madlib pull request #250: MLP: Allow one-hot encoded dependent var for class...
Posted by njayaram2 <gi...@git.apache.org>.
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/250#discussion_r178168247
--- Diff: src/ports/postgres/modules/convex/mlp_igd.py_in ---
@@ -667,7 +678,8 @@ def _validate_dependent_var(source_table, dependent_varname,
if is_classification:
# Currently, classification doesn't accept an
# array for dep type in IGD
- _assert("[]" not in expr_type and expr_type in classification_types,
+ _assert(("[]" in expr_type and expr_type[:-2] in int_types) \
--- End diff --
- One-hot encoding is normally supposed to be an integer array, hence was checking only for int type. But, that assumption might be too strict, so will make changes to allow for other numeric types too.
- Good catch. It might be a good idea to disallow anything greater than a 1-D array. Will make an error check for the same.
---
[GitHub] madlib issue #250: MLP: Allow one-hot encoded dependent var for classificati...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/250
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/420/
---