Implement Default fmod Functionality for Base Tensor Type #46

[nahuakang]

Hi y'all,

This is another attempt to try pull request and to ask for feedback. Both fmod() and fmod()_ methods are added together with corresponding unit tests. However, fmod() is slightly different from the one in PyTorch as it does not contain the optional parameter out.

Thank you for your time and feedback!

Best,

Nahua

[aiorla]

Thank you @nahuakang! Can you solve first the style issues that Travis reported? (you can reproduce them locally with pytest && pytest --flake8 or using make test in the latest master)

Also, I think that PR #219 is also trying to tackle #46 but I'm not sure about its status. (ping: @radicalrafi)

[ghost]

PR #219 is supposed to tackle this one will submit final edits today .

[nahuakang]

@aiorla Thank you for your feedback. There seems to be a lot of Flake8 errors from 1113-1145. Is this caused by some actions from me? And is the best way to fix style issues just to do it manually?

@radicalrafi Thanks. So I should stop working on this issue now?

[ghost]

P.S I'm running in some trouble with my local env can't seem to fix if yours is good to go I have closed my OLD PR #219

[ghost]

You might need to run flake8 locally or trace the Travis Log and fix the syntax issues .

[nahuakang]

@radicalrafi @aiorla Thank you both for your help, and I manually fixed the line and space issues. Travis-CI builds successfully now.

[bharathgs]

@nahuakang this looks great, could you please rewrite this in math.py and add the respective methods in tensor.py? (sorry to ask these changes now! 😬) but otherwise we are good to go.
Since this is calculating the element-wise remainder of the division, it better suites over there.
@aiorla

[nahuakang]

@bharathgs If I'm understanding you correctly, your advice is:

1. add fmod() in math.py, and call syft.fmod() in tensor.py instead?
2. _fmod() stays in tensor.py?

[aiorla]

@bharathgs I won't comment on the function location because I'm still not sure about what rules are we following. (but you may be right)

My feedback about the PR (I was waiting Travis to check it):

• I think we need to check that divisor is not encrypted (if it's a tensor).
• I'm not sure why only the first dimension is checked to decide if they have the same shape (self.shape()[0]).

PS: I think PyTorch is a bit inconsistent in its definition. It's not clear if we need the same shape or just for them to be broadcastable... 😟

[bharathgs]

@aiorla yes you are right (about the feedback)

@nahuakang yes you are right as well. fmod moves to math.py

[nahuakang]

@aiorla @bharathgs
I modified fmod() and put it under math.py, and modified fmod_() under tensor.py according to comment from @bharathgs .
While the comment from @aiorla is valid and important, I simply edited the documentation for fmod() and fmod_() this time according to another similar method remainder() (and remainder_()) in tensor.py. So if the divisor cannot be broadcasted due to conflicting shape, numpy.fmod will throw the error.
Let me know if you have any feedback. Thanks!

[aiorla]

Looks 👍 to me. Thanks for your patience @nahuakang!