gaturchenko commented on code in PR #2542: URL: https://github.com/apache/systemds/pull/2542#discussion_r3657166290
########## src/test/java/org/apache/sysds/test/functions/unique/LibMatrixSketchUniqueParallelTest.java: ########## Review Comment: This test file is rather inconsistent with the general structure of function tests. The biggest issue is that you don't actually invoke a DML script but rather call the instruction directly. So essentially instead of end-to-end you have unit tests, and we generally prefer the former. Therefore, please have another look at how existing unique tests are implemented. If necessary, you can extend the `UniqueBase` class. Regarding the tests that constrain the memory budget to trigger the batched path, you can in principle leave them as they are, but I'd still appreciate if you could find a way to make them DML-based as well. -- 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]
