mboehm7 commented on pull request #1133: URL: https://github.com/apache/systemds/pull/1133#issuecomment-751864222
LGTM - welcome to SystemDS and thanks for the patch @ywcb00. During the merge, I made a number of smaller corrections - don't worry about them but please keep them in mind for the future. First, avoid git-merges and an unnecessarily fine-grained commit log as this creates lots of conflicts and issues during the final merge (I actually took the code, and created a commit from scratch under your name). You can rebase to master if necessary. Second, regarding formatting, we use 2-space indentation for scripts, but tab-indentation for Java code (your code used 2-spaces there too). Similarly, try to avoid long generic variable names (e.g., execution_context, matrix_object_1). Third, I fixed the test to avoid unnecessary state updates (test_name), use spark execution type for the reference runs too, and moved this test to the primitives package (which is already part of our github actions). Fourth, I changed the handling of expected federated inputs to throw an exception on unsupported combinations to avoid silent errors of not producing the output. If you want to continue with the quaternary operations, I would recommend to (1) handle the remaining loss functions wsloss and wsigmoid, then (2) the factor updates wdivmm and wumm. Once we have them all, we can add respective algorithm-level tests like ALS-CG. Thanks for your contributions. Btw, please add you chosen commit email address (in your commit message) with your github handle (e.g., as a secondary email address), otherwise your account it not properly linked to the commit and you don't show up as a contributor. ---------------------------------------------------------------- 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]
