corepointer commented on pull request #1275: URL: https://github.com/apache/systemds/pull/1275#issuecomment-843062760
> > > Hello and thank you for your effort in this PR! > > > In general I agree with Sebastian, it looks good, but if I may chime in with additional questions: > > > Why is Pillow used? I'm sure there are image manipulation libraries for R and Java that could be used. > > > I guess the use of Pillow is the reason for the additional resource files and the methods in TestUtils? That (and the mentioned precision issues) could be avoided by using an R or Java library for comparison. > > > jm2c ;-) > > > Mark > > > > > > I use Pillow as a reference because one of the reasons I added the builtins is to reproduce the results of AutoAugment (https://arxiv.org/abs/1805.09501) and they use Pillow. OK, wasn't aware of that. > > I don't think using Pillow is an issues, since it is only used in tests and not actually added to the system, since all the results seems to be added as references. Therefore nothing will not be included as a resource in the system. Spotted a double negation there :D Was that your intention? It actually fits well as there are resource files included. > > That said it would be better with the actual expected results (calculated and verified not using our system), than results where we have to consider rounding errors from floating point to uint. but i consider this out of scope of this project, since the methods are tested and verified sufficiently. Another suggestion that'd be out of scope: Comparing to AutoAugment is out of scope of a unit test and should go into a dedicated script that can happily use Pillow and everything that is needed to run AutoAugment. For a unit test it is sufficient to have the correct functionality of our code confirmed. But that is all just expressing my opinion again - no action needed on your part. Mark -- 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]
