mboehm7 commented on pull request #1360: URL: https://github.com/apache/systemds/pull/1360#issuecomment-895553192
LGTM - thanks for creating the builtin function and test @tim-sagaster. Also thanks for the original algorithm @iyounus. During the merge I made a few tweaks though, which I mention here just for completeness. First, I fixed the test to use a relatively large epsilon so it passes (so far it compared equivalence of doubles which even for tiny discrepancies would fail; it seems ok, but there are a few values where the absolute error goes up to 10% of the value range; we will follow up to understand the algorithm-level differences; also, for iterative algorithms it's not useful to force all operations into distributed Spark ops so nothing to worry there). Second, I left a few TODOs to consolidate the distance computation with the respective builtin and compare against R instead of inlining expected outputs. Finally, I also fixed some formatting and stdout issues and unnecessary imports in the test. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
