[GitHub] madlib pull request #268: DT: Don't use NULL value to get dep_var type
Github user asfgit closed the pull request at: https://github.com/apache/madlib/pull/268 ---
[GitHub] madlib pull request #272: MLP: Add momentum and nesterov to gradient updates...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/272#discussion_r192246910
--- Diff: doc/design/modules/neural-network.tex ---
@@ -117,6 +117,24 @@ \subsubsection{Backpropagation}
\[\boxed{\delta_{k}^j = \sum_{t=1}^{n_{k+1}} \left( \delta_{k+1}^t \cdot
u_{k}^{jt} \right) \cdot \phi'(\mathit{net}_{k}^j)}\]
where $k = 1,...,N-1$, and $j = 1,...,n_{k}$.
+\paragraph{Momentum updates.}
+Momentum\cite{momentum_ilya}\cite{momentum_cs231n} can help accelerate
learning and avoid local minima when using gradient descent. We also support
nesterov's accelarated gradient due to its look ahead characteristics. \\
+Here we need to introduce two new variables namely velocity and momentum.
momentum must be in the range 0 to 1, where 0 means no momentum. The momentum
value is responsible for damping the velocity and is analogous to the
coefficient of friction. \\
+In classical momentum you first correct the velocity and step with that
velocity, whereas in Nesterov momentum you first step in the velocity direction
then make a correction to the velocity vector based on the new location. \\
+Classic momentum update
+\[\begin{aligned}
+\mathit{v} \set \mathit{mu} * \mathit{v} - \eta * \mathit{gradient}
\text{ (velocity update)} \\ % $\eta$,\\ * $\nabla f(u)$
+\mathit{u} \set \mathit{u} + \mathit{v} \\
+\end{aligned}\]
+
+Nesterov momentum update
+\[\begin{aligned}
+\mathit{u} \set \mathit{u} + \mathit{mu} * \mathit{v} \text{
(nesterov's initial coefficient update )} \\
+\mathit{v} \set \mathit{mu} * \mathit{v} - \eta * \mathit{gradient}
\text{ (velocity update)} \\ % $\eta$,\\ * $\nabla f(u)$
+\mathit{u} \set \mathit{u} - \eta * \mathit{gradient} \\
--- End diff --
Can we left-align these and other momentum related equations?
---
[GitHub] madlib pull request #272: MLP: Add momentum and nesterov to gradient updates...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/272#discussion_r192246463
--- Diff: doc/design/modules/neural-network.tex ---
@@ -117,6 +117,24 @@ \subsubsection{Backpropagation}
\[\boxed{\delta_{k}^j = \sum_{t=1}^{n_{k+1}} \left( \delta_{k+1}^t \cdot
u_{k}^{jt} \right) \cdot \phi'(\mathit{net}_{k}^j)}\]
where $k = 1,...,N-1$, and $j = 1,...,n_{k}$.
+\paragraph{Momentum updates.}
+Momentum\cite{momentum_ilya}\cite{momentum_cs231n} can help accelerate
learning and avoid local minima when using gradient descent. We also support
nesterov's accelarated gradient due to its look ahead characteristics. \\
+Here we need to introduce two new variables namely velocity and momentum.
momentum must be in the range 0 to 1, where 0 means no momentum. The momentum
value is responsible for damping the velocity and is analogous to the
coefficient of friction. \\
--- End diff --
We only describe the range for momentum, but nothing about velocity before
the next line begins. Can we say a bit about what velocity is before we
describe how it is used?
---
[GitHub] madlib pull request #272: MLP: Add momentum and nesterov to gradient updates...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/272#discussion_r192251198
--- Diff: doc/design/modules/neural-network.tex ---
@@ -117,6 +117,24 @@ \subsubsection{Backpropagation}
\[\boxed{\delta_{k}^j = \sum_{t=1}^{n_{k+1}} \left( \delta_{k+1}^t \cdot
u_{k}^{jt} \right) \cdot \phi'(\mathit{net}_{k}^j)}\]
--- End diff --
Please add your name to the list of authors in the beginning of the file.
Maybe add the history for this file as well?
---
[GitHub] madlib pull request #272: MLP: Add momentum and nesterov to gradient updates...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/272#discussion_r192245605
--- Diff: doc/design/modules/neural-network.tex ---
@@ -117,6 +117,24 @@ \subsubsection{Backpropagation}
\[\boxed{\delta_{k}^j = \sum_{t=1}^{n_{k+1}} \left( \delta_{k+1}^t \cdot
u_{k}^{jt} \right) \cdot \phi'(\mathit{net}_{k}^j)}\]
where $k = 1,...,N-1$, and $j = 1,...,n_{k}$.
+\paragraph{Momentum updates.}
+Momentum\cite{momentum_ilya}\cite{momentum_cs231n} can help accelerate
learning and avoid local minima when using gradient descent. We also support
nesterov's accelarated gradient due to its look ahead characteristics. \\
+Here we need to introduce two new variables namely velocity and momentum.
momentum must be in the range 0 to 1, where 0 means no momentum. The momentum
value is responsible for damping the velocity and is analogous to the
coefficient of friction. \\
+In classical momentum you first correct the velocity and step with that
velocity, whereas in Nesterov momentum you first step in the velocity direction
then make a correction to the velocity vector based on the new location. \\
--- End diff --
`step with that velocity` is a little confusing to me. Do we have some
source where it is defined this way?
If it's any better, can we use the following text to say what the
difference between momentum and NAG is (source is
http://www.cs.utoronto.ca/~ilya/pubs/ilya_sutskever_phd_thesis.pdf):
```
... the key difference between momentum and Nesterovâs
accelerated gradient is that momentum computes the gradient before applying
the velocity, while Nesterovâs
accelerated gradient computes the gradient after doing so.
```
If `step with that velocity` is a standard way of defining it, then I am
okay with it.
This comment applies to user and online docs too.
---
[GitHub] madlib pull request #272: MLP: Add momentum and nesterov to gradient updates...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/272#discussion_r192248589
--- Diff: doc/design/modules/neural-network.tex ---
@@ -117,6 +117,24 @@ \subsubsection{Backpropagation}
\[\boxed{\delta_{k}^j = \sum_{t=1}^{n_{k+1}} \left( \delta_{k+1}^t \cdot
u_{k}^{jt} \right) \cdot \phi'(\mathit{net}_{k}^j)}\]
where $k = 1,...,N-1$, and $j = 1,...,n_{k}$.
+\paragraph{Momentum updates.}
+Momentum\cite{momentum_ilya}\cite{momentum_cs231n} can help accelerate
learning and avoid local minima when using gradient descent. We also support
nesterov's accelarated gradient due to its look ahead characteristics. \\
+Here we need to introduce two new variables namely velocity and momentum.
momentum must be in the range 0 to 1, where 0 means no momentum. The momentum
value is responsible for damping the velocity and is analogous to the
coefficient of friction. \\
+In classical momentum you first correct the velocity and step with that
velocity, whereas in Nesterov momentum you first step in the velocity direction
then make a correction to the velocity vector based on the new location. \\
+Classic momentum update
+\[\begin{aligned}
+\mathit{v} \set \mathit{mu} * \mathit{v} - \eta * \mathit{gradient}
\text{ (velocity update)} \\ % $\eta$,\\ * $\nabla f(u)$
+\mathit{u} \set \mathit{u} + \mathit{v} \\
+\end{aligned}\]
+
+Nesterov momentum update
+\[\begin{aligned}
+\mathit{u} \set \mathit{u} + \mathit{mu} * \mathit{v} \text{
(nesterov's initial coefficient update )} \\
+\mathit{v} \set \mathit{mu} * \mathit{v} - \eta * \mathit{gradient}
\text{ (velocity update)} \\ % $\eta$,\\ * $\nabla f(u)$
--- End diff --
Instead of `gradient`, can we replace it with the actual math notation?
I think the gradient at this line is: `\frac{\partial f}{\partial
u_{k-1}^{sj}}`
The gradient in the next line should be w.r.t `v` instead of `u`.
---
REMINDER: Apache EU Roadshow 2018 in Berlin is less than 2 weeks away!
Hello Apache Supporters and Enthusiasts This is a reminder that our Apache EU Roadshow in Berlin is less than two weeks away and we need your help to spread the word. Please let your work colleagues, friends and anyone interested in any attending know about our Apache EU Roadshow event. We have a great schedule including tracks on Apache Tomcat, Apache Http Server, Microservices, Internet of Things (IoT) and Cloud Technologies. You can find more details at the link below: https://s.apache.org/0hnG Ticket prices will be going up on 8^th June 2018, so please make sure that you register soon if you want to beat the price increase. https://foss-backstage.de/tickets Remember that registering for the Apache EU Roadshow also gives you access to FOSS Backstage so you can attend any talks and workshops from both conferences. And don’t forget that our Apache Lounge will be open throughout the whole conference as a place to meet up, hack and relax. We look forward to seeing you in Berlin! Thanks Sharan Foga, VP Apache Community Development http://apachecon.com/ @apachecon PLEASE NOTE: You are receiving this message because you are subscribed to a user@ or dev@ list of one or more Apache Software Foundation projects.
[GitHub] madlib issue #272: MLP: Add momentum and nesterov to gradient updates.
Github user asfgit commented on the issue: https://github.com/apache/madlib/pull/272 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/madlib-pr-build/493/ ---
[GitHub] madlib issue #271: Madpack: Make install, reinstall and upgrade atomic
Github user asfgit commented on the issue: https://github.com/apache/madlib/pull/271 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/madlib-pr-build/492/ ---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/271#discussion_r192186350
--- Diff: src/madpack/madpack.py ---
@@ -131,10 +141,73 @@ def _get_relative_maddir(maddir, port):
return maddir
#
--
+def _cleanup_comments_in_sqlfile(output_filename, upgrade):
+"""
+@brief Remove comments in the sql script
+"""
+if not upgrade:
+with open(output_filename, 'r+') as output_filehandle:
+full_sql = output_filehandle.read()
+pattern =
re.compile(r"""(/\*(.|[\r\n])*?\*/)|(--(.*|[\r\n]))""")
+res = ''
+lines = re.split(r'[\r\n]+', full_sql)
+for line in lines:
+tmp = line
+if not tmp.strip().startswith("E'"):
+line = re.sub(pattern, '', line)
+res += line + '\n'
+full_sql = res.strip()
+full_sql = re.sub(pattern, '', full_sql).strip()
+# Re-write the cleaned-up sql to a new file. Python does not let us
+# erase all the content of a file and rewrite the same file again.
+cleaned_output_filename = output_filename+'.tmp'
+with open(cleaned_output_filename, 'w') as output_filehandle:
+_write_to_file(output_filehandle, full_sql)
+# Move the cleaned output file to the old one.
+os.rename(cleaned_output_filename, output_filename)
+
+def _run_m4_and_append(schema, maddir_mod_py, module, sqlfile,
+ output_filehandle, pre_sql=None):
+"""
+Function to process a sql file with M4.
+"""
+# Check if the SQL file exists
+if not os.path.isfile(sqlfile):
+error_(this, "Missing module SQL file (%s)" % sqlfile, False)
--- End diff --
We removed the error message from `ValueError`, while we still print the
message from `error_()`.
---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request: https://github.com/apache/madlib/pull/271#discussion_r192174800 --- Diff: src/madpack/upgrade_util.py --- @@ -1299,18 +1303,19 @@ def _clean_function(self): pattern = re.compile(r"""CREATE(\s+)FUNCTION""", re.DOTALL | re.IGNORECASE) self._sql = re.sub(pattern, 'CREATE OR REPLACE FUNCTION', self._sql) -def cleanup(self, sql): +def cleanup(self, sql, algoname): --- End diff -- Good catch, this is dead code. ---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request: https://github.com/apache/madlib/pull/271#discussion_r192193089 --- Diff: src/madpack/madpack.py --- @@ -559,71 +650,59 @@ def _db_rename_schema(from_schema, to_schema): # -- -def _db_create_schema(schema): +def _db_create_schema(schema, is_schema_in_db, filehandle): """ Create schema @param from_schema name of the schema to rename +@param is_schema_in_db flag to indicate if schema is already present @param to_schema new name for the schema """ -info_(this, "> Creating %s schema" % schema.upper(), True) --- End diff -- This is useful and it was preserved in `main`. ---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/271#discussion_r192205268
--- Diff: src/madpack/madpack.py ---
@@ -987,275 +1276,42 @@ def main(argv):
error_(this, "Missing -p/--platform parameter.", True)
if not con_args:
error_(this, "Unknown problem with database connection string:
%s" % con_args, True)
+# Completed "Get and validate arguments"
-
# COMMAND: version
if args.command[0] == 'version':
_print_revs(rev, dbrev, con_args, schema)
-# COMMAND: uninstall/reinstall
-if args.command[0] in ('uninstall', 'reinstall'):
-if get_rev_num(dbrev) == [0]:
-info_(this, "Nothing to uninstall. No version found in schema
%s." % schema.upper(), True)
-return
-
-# Find any potential data to lose
-affected_objects = _internal_run_query("""
-SELECT
-n1.nspname AS schema,
-relname AS relation,
-attname AS column,
-typname AS type
-FROM
-pg_attribute a,
-pg_class c,
-pg_type t,
-pg_namespace n,
-pg_namespace n1
-WHERE
-n.nspname = '%s'
-AND t.typnamespace = n.oid
-AND a.atttypid = t.oid
-AND c.oid = a.attrelid
-AND c.relnamespace = n1.oid
-AND c.relkind = 'r'
-ORDER BY
-n1.nspname, relname, attname, typname""" % schema.lower(),
True)
-
-info_(this, "*** Uninstalling MADlib ***", True)
-info_(this,
"***",
True)
-info_(this, "* Schema %s and all database objects depending on it
will be dropped!" % schema.upper(), True)
-if affected_objects:
-info_(this, "* If you continue the following data will be lost
(schema : table.column : type):", True)
-for ao in affected_objects:
-info_(this, '* - ' + ao['schema'] + ' : ' + ao['relation']
+ '.' +
- ao['column'] + ' : ' + ao['type'], True)
-info_(this,
"***",
True)
-info_(this, "Would you like to continue? [Y/N]", True)
-go = raw_input('>>> ').upper()
-while go != 'Y' and go != 'N':
-go = raw_input('Yes or No >>> ').upper()
-
-# 2) Do the uninstall/drop
-if go == 'N':
-info_(this, 'No problem. Nothing dropped.', True)
-return
-
-elif go == 'Y':
-info_(this, "> dropping schema %s" % schema.upper(), verbose)
-try:
-_internal_run_query("DROP SCHEMA %s CASCADE;" % (schema),
True)
-except:
-error_(this, "Cannot drop schema %s." % schema.upper(),
True)
-
-info_(this, 'Schema %s (and all dependent objects) has been
dropped.' % schema.upper(), True)
-info_(this, 'MADlib uninstalled successfully.', True)
-
-else:
-return
-
-# COMMAND: install/reinstall
-if args.command[0] in ('install', 'reinstall'):
-# Refresh MADlib version in DB, None for GP/PG
-if args.command[0] == 'reinstall':
-print "Setting MADlib database version to be None for
reinstall"
-dbrev = None
-
-info_(this, "*** Installing MADlib ***", True)
-
-# 1) Compare OS and DB versions.
-# noop if OS <= DB.
-_print_revs(rev, dbrev, con_args, schema)
-if is_rev_gte(get_rev_num(dbrev), get_rev_num(rev)):
-info_(this, "Current MADlib version already up to date.", True)
-return
-# proceed to create objects if nothing installed in DB
-elif dbrev is None:
-pass
-# error and refer to upgrade if OS > DB
-else:
-error_(this, """Aborting installation: existing MADlib version
detected in {0} schema
-To upgrade the {0} schema to MADlib v{1} please run
the following command:
-madpack upgrade -s {0} -p {2} [-c ...]
-""".format(schema, rev, portid), True)
-
-# 2) Run installation
-try:
-_plpy_check(py_min_ver)
-_db_install(schema, dbrev, args.testcase)
-except:
-error_(this, "MADlib
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request: https://github.com/apache/madlib/pull/271#discussion_r192175486 --- Diff: src/madpack/upgrade_util.py --- @@ -1299,18 +1303,19 @@ def _clean_function(self): pattern = re.compile(r"""CREATE(\s+)FUNCTION""", re.DOTALL | re.IGNORECASE) self._sql = re.sub(pattern, 'CREATE OR REPLACE FUNCTION', self._sql) -def cleanup(self, sql): +def cleanup(self, sql, algoname): """ @brief Entry function for cleaning the sql script """ self._sql = sql -self._clean_comment() -self._clean_type() -self._clean_cast() -self._clean_operator() -self._clean_opclass() -self._clean_aggregate() -self._clean_function() +if algoname not in self.get_change_handler().newmodule: --- End diff -- This if check was originally done in `madpack.py` before `cleanup()` function was called. We moved it to this function instead. Will add the comment, but keep the if logic as is, since we will anyway have to return at the end. ---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/271#discussion_r192204168
--- Diff: src/madpack/madpack.py ---
@@ -824,6 +873,246 @@ def parse_arguments():
# Get the arguments
return parser.parse_args()
+def run_install_check(args, testcase):
+schema = args['schema']
+dbrev = args['dbrev']
+# 1) Compare OS and DB versions. Continue if OS = DB.
+if get_rev_num(dbrev) != get_rev_num(rev):
+_print_revs(rev, dbrev, con_args, schema)
+info_(this, "Versions do not match. Install-check stopped.", True)
+return
+
+# Create install-check user
+test_user = ('madlib_' +
+ rev.replace('.', '').replace('-', '_') +
+ '_installcheck')
+try:
+_internal_run_query("DROP USER IF EXISTS %s;" % (test_user), False)
+except:
+_internal_run_query("DROP OWNED BY %s CASCADE;" % (test_user),
True)
+_internal_run_query("DROP USER IF EXISTS %s;" % (test_user), True)
+_internal_run_query("CREATE USER %s;" % (test_user), True)
+
+_internal_run_query("GRANT USAGE ON SCHEMA %s TO %s;" % (schema,
test_user), True)
+
+# 2) Run test SQLs
+info_(this, "> Running test scripts for:", verbose)
+
+caseset = (set([test.strip() for test in testcase.split(',')])
+ if testcase != "" else set())
+
+modset = {}
+for case in caseset:
+if case.find('/') > -1:
+[mod, algo] = case.split('/')
+if mod not in modset:
+modset[mod] = []
+if algo not in modset[mod]:
+modset[mod].append(algo)
+else:
+modset[case] = []
+
+# Loop through all modules
+for moduleinfo in portspecs['modules']:
+
+# Get module name
+module = moduleinfo['name']
+
+# Skip if doesn't meet specified modules
+if modset is not None and len(modset) > 0 and module not in modset:
+continue
+# JIRA: MADLIB-1078 fix
+# Skip pmml during install-check (when run without the -t option).
+# We can still run install-check on pmml with '-t' option.
+if not modset and module in ['pmml']:
+continue
+info_(this, "> - %s" % module, verbose)
+
+# Make a temp dir for this module (if doesn't exist)
+cur_tmpdir = tmpdir + '/' + module + '/test' # tmpdir is a global
variable
+_make_dir(cur_tmpdir)
+
+# Find the Python module dir (platform specific or generic)
+if os.path.isdir(maddir + "/ports/" + portid + "/" + dbver +
"/modules/" + module):
+maddir_mod_py = maddir + "/ports/" + portid + "/" + dbver +
"/modules"
+else:
+maddir_mod_py = maddir + "/modules"
+
+# Find the SQL module dir (platform specific or generic)
+if os.path.isdir(maddir + "/ports/" + portid + "/modules/" +
module):
+maddir_mod_sql = maddir + "/ports/" + portid + "/modules"
+else:
+maddir_mod_sql = maddir + "/modules"
+
+# Prepare test schema
+test_schema = "madlib_installcheck_%s" % (module)
+_internal_run_query("DROP SCHEMA IF EXISTS %s CASCADE; CREATE
SCHEMA %s;" %
+(test_schema, test_schema), True)
+_internal_run_query("GRANT ALL ON SCHEMA %s TO %s;" %
+(test_schema, test_user), True)
+
+# Switch to test user and prepare the search_path
+pre_sql = '-- Switch to test user:\n' \
+ 'SET ROLE %s;\n' \
+ '-- Set SEARCH_PATH for install-check:\n' \
+ 'SET search_path=%s,%s;\n' \
+ % (test_user, test_schema, schema)
+
+# Loop through all test SQL files for this module
+sql_files = maddir_mod_sql + '/' + module + '/test/*.sql_in'
+for sqlfile in sorted(glob.glob(sql_files), reverse=True):
+algoname = os.path.basename(sqlfile).split('.')[0]
+# run only algo specified
+if (module in modset and modset[module] and
+algoname not in modset[module]):
+continue
+
+# Set file names
+tmpfile = cur_tmpdir + '/' + os.path.basename(sqlfile) + '.tmp'
+logfile = cur_tmpdir + '/' + os.path.basename(sqlfile) + '.log'
+
+# If there is no problem with the SQL file
+milliseconds = 0
+
+# Run the SQL
+run_start = datetime.datetime.now()
+
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request:
https://github.com/apache/madlib/pull/271#discussion_r192182069
--- Diff: src/madpack/madpack.py ---
@@ -95,6 +95,16 @@ def _internal_run_query(sql, show_error):
return run_query(sql, con_args, show_error)
#
--
+def _write_to_file(handle, sql, show_error=False):
+handle.write(sql)
+handle.write('\n')
+#
--
+
+
+def _merge_to_file(handle, other_file, show_error=False):
--- End diff --
It was used very early in the story I guess, have removed it now.
---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request: https://github.com/apache/madlib/pull/271#discussion_r192193755 --- Diff: src/madpack/madpack.py --- @@ -824,6 +873,246 @@ def parse_arguments(): # Get the arguments return parser.parse_args() +def run_install_check(args, testcase): +schema = args['schema'] --- End diff -- This is install check code which is not refactored in this commit, and should be done as part of a different commit. We only moved `install-check` specific code from `main` to this function, since `main` was very huge. ---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request: https://github.com/apache/madlib/pull/271#discussion_r192181634 --- Diff: src/madpack/madpack.py --- @@ -95,6 +95,16 @@ def _internal_run_query(sql, show_error): return run_query(sql, con_args, show_error) # -- +def _write_to_file(handle, sql, show_error=False): --- End diff -- Moved this function to `madlib/utilities.py`. ---
[GitHub] madlib pull request #271: Madpack: Make install, reinstall and upgrade atomi...
Github user njayaram2 commented on a diff in the pull request: https://github.com/apache/madlib/pull/271#discussion_r192178673 --- Diff: src/madpack/utilities.py --- @@ -33,6 +33,23 @@ this = os.path.basename(sys.argv[0])# name of this script +class AtomicFileOpen: --- End diff -- It's not used, will remove it. ---
[GitHub] madlib issue #271: Madpack: Make install, reinstall and upgrade atomic
Github user njayaram2 commented on the issue: https://github.com/apache/madlib/pull/271 Thank you for the comments @kaknikhil , will address them. ---
