Pysyft: Small rounding errors in handcrafted Conv2d and Pool2d Layers
Describe the bug
There is a small rounding error in these layers, as discussed in https://github.com/OpenMined/PySyft/pull/2896/files/fa21f480c8bfe4196dac2f911cd938db7ff96f03#diff-369e971f29cf5226c9b64f75072095ac
To Reproduce
Run the following unit tests
Expected behavior
Ideally there would be 0 rounding error, but this might be a PyTorch issue not us.
iamtrask
All 8 comments
I investigated this a bit and it seems we need to do a direct summation instead of calculating sum of sums. The following modifications are required:
By making the above changes, we get exactly the same output for both the pooling implementations. However, a rounding error is still present for convolution possibly due to floating-point arithmetic as indicated here.
arshjot
on 21 Jan 2020
@arshjot Thanks for looking into it! Could you submit a PR with those changes?
karlhigley
on 21 Jan 2020
Yes, I'll be happy to do so!
arshjot
on 21 Jan 2020
Incredible work!
Sent from my iPhone
On 21 Jan 2020, at 19:20, Arshjot Singh Khehra notifications@github.com wrote:

Yes, I'll be happy to do so!—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
iamtrask
on 21 Jan 2020
As reported - there is still rounding error in the Conv2d implementation. This issue should stay open until it is resolved. I have spent a few more hours looking into it and haven't found anything yet.
iamtrask
on 24 Jan 2020
That's strange, it looks like the PR solved it and chnaged the test form approx to exact check didn't it?
LaRiffle
on 24 Jan 2020
The issue is solved for the pooling implementation but not for Conv2d. The Conv2d implementation also had the same discrepancy but even after rectifying it we do not get an exact match. I guess I should have specified that the PR solves part of this issue.
arshjot
on 24 Jan 2020
This issue was fixed by #2964 (for the conv2d layer) and #2945 (for the pooling layer)
gmuraru
on 27 Jan 2020
Related issues
swaroopch
·
4Comments
s-marta
·
4Comments
MetaT1an
·
3Comments
beatrizsmg
·
4Comments
mgale694
·
3Comments
Most helpful comment
I investigated this a bit and it seems we need to do a direct summation instead of calculating sum of sums. The following modifications are required:
conv.py- change.sum(3).sum(3)to.sum((3, 4))pool.py- change.sum(2).sum(2)to.sum((2, 3))By making the above changes, we get exactly the same output for both the pooling implementations. However, a rounding error is still present for convolution possibly due to floating-point arithmetic as indicated here.