kaknikhil commented on a change in pull request #522:
URL: https://github.com/apache/madlib/pull/522#discussion_r505089686
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_wrapper.py_in
##########
@@ -248,7 +249,7 @@ def parse_optimizer(compile_dict):
opt_split = compile_dict['optimizer'].split('(')
opt_name = opt_split[0]
optimizers = get_optimizers()
- _assert(opt_name in optimizers,
+ _assert(opt_name.lower() in [o.lower() for o in optimizers.keys()],
Review comment:
what if opt_name is None ?
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras.py_in
##########
@@ -44,7 +39,15 @@ from utilities.utilities import madlib_version
from utilities.utilities import unique_string
from utilities.validate_args import get_expr_type
from utilities.control import MinWarning
+
import tensorflow as tf
+from tensorflow import keras
Review comment:
We weren't explicitly importing keras before, is this for the case when
user has a `keras` dependency in their model json ?
##########
File path: src/ports/postgres/modules/deep_learning/test/madlib_keras_fit.sql_in
##########
@@ -397,36 +397,3 @@ SELECT assert(
class_values = '{NULL,0,1,4,5}',
'Keras model output Summary Validation failed. Actual:' ||
__to_char(summary))
FROM (SELECT * FROM keras_saved_out_summary) summary;
-
Review comment:
I think it will help to add a note to the commit message explaining why
we removed these tests
----------------------------------------------------------------
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:
us...@infra.apache.org
