[GitHub] [madlib] jingyimei commented on a change in pull request #382: DL: Naming improvements for dependent and independent varname

[GitBox <gi...@apache.org>]

jingyimei commented on a change in pull request #382: DL: Naming improvements for dependent and independent varname
URL: https://github.com/apache/madlib/pull/382#discussion_r280642965
 
 

 ##########
 File path: src/ports/postgres/modules/deep_learning/madlib_keras.py_in
 ##########
 @@ -209,6 +211,10 @@ def fit(schema_madlib, source_table, model, dependent_varname,
         fit_validator.source_summary_table, NORMALIZING_CONST_COLNAME)
     dep_vartype = plpy.execute("SELECT {0} AS dep FROM {1}".format(
         DEPENDENT_VARTYPE_COLNAME, fit_validator.source_summary_table))[0]['dep']
+    dependent_varname_in_source_table = plpy.execute("SELECT {0} FROM {1}".format(
+        'dependent_varname', fit_validator.source_summary_table))[0]['dependent_varname']
+    independent_varname_in_source_table = plpy.execute("SELECT {0} FROM {1}".format(
+        'independent_varname', fit_validator.source_summary_table))[0]['independent_varname']
 
 Review comment:
   I did look at here and try to make them variable. The problem here is in `madlib_keras_helper.py_in` https://github.com/apache/madlib/blob/master/src/ports/postgres/modules/deep_learning/madlib_keras_helper.py_in#L30, we had a comment saying those are columns in model summary table, while here it is actually from source summary table (summary table from minibatch preprocessor). We already use those variables in a confusing way. Besides, we don't have a global variable system for minibatch summary table(see https://github.com/apache/madlib/blob/master/src/ports/postgres/modules/utilities/minibatch_preprocessing.py_in#L495), and we use this kind of hard quote in many other places(example https://github.com/apache/madlib/blob/master/src/ports/postgres/modules/deep_learning/madlib_keras.py_in#L211). If we want to resolve this thoroughly, we need to build similar system to minibatch summary table and remove those hard quote.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services