njayaram2 commented on a change in pull request #370: DL: Support response and prob prediction outputs URL: https://github.com/apache/madlib/pull/370#discussion_r276836606
########## File path: src/ports/postgres/modules/deep_learning/madlib_keras_helper.py_in ########## @@ -0,0 +1,96 @@ +# coding=utf-8 Review comment: Agree and disagree. `predict_input_params.py_in` makes sense, and I have made the necessary changes for that. But for the other function, creating a new file called `all_things_numpy` may not be a good idea since we do have a lot of functions that use numpy in a different file. So that filename will be a misnomer. If we decide to name it something else, then there will be just that one function in that file, and we will also have to create a new file for the colnames variables. I'd rather have them both together in `madlib_keras_helper.py_in`. IMO, If we add more functions in future PRs, we should create appropriate files if it makes sense, else it's ok to add it in the helper file if we cannot classify it cleanly to a separate file. ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
