Pysyft: Handcrafted RNNs are not Tested At All

[iamtrask]

Describe the bug
This whole file seems to be largely un-tested by our unit testing suite.
https://github.com/OpenMined/PySyft/blob/master/syft/frameworks/torch/nn/rnn.py

Expected behavior
This file should have 100% test coverage.

Make sure to uncomment the codecov omit flag for this file https://github.com/OpenMined/PySyft/pull/2896#pullrequestreview-344948386

[ribhu97]

I can take this up

[jimboH]

Hi, I would like to know if this is still an active issue. If it is, could I take this issue?

[iamtrask]

It is and you may!

Sent from my iPhone

On 22 Feb 2020, at 17:37, jimboH [e-mail] wrote:


Hi, I would like to know if this is still an active issue. If it is, could I take this issue?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[AniTho]

Hi, I would like to work on this issue. Is it still active?

[jimboH]

@iamtrask thank you! I will work on it.

[AniTho]

@karlhigley thank you! I will start working on it.

[iamtrask]

Sounds like you should both work together on it.

Sent from my iPhone

On 23 Feb 2020, at 16:46, AniTho [e-mail] wrote:


@karlhigley thank you! I will start working on it.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.

[AniTho]

@iamtrask I have to implement test case as you have done for conv2d which is by creating a sample object using torch nn and one of the custom implementation and then finding maximum absolute difference in the two model output?

[AniTho]

@jimboH Can you share anything through which I can contact you to collaborate?

[jimboH]

@AniTho Thank you! I got some troubles with rounding while testing the handcrafted RNNs but finally solved it. Would you take a look at the test_rnn.py file to see if it really test all the RNNs?

[NileshPant1999]

Hi, I would like to work on this issue. Is it still active?

[AniTho]

@jimboH yes I'd like to see the code.

[karlhigley]

See #3092 for the current state of the testing effort.

[sachin-101]

3092 is merged. Do more tests need to be added ? @iamtrask @karlhigley

If not, then you can close this issue.