Repository: OpenMined/PySyft
Pull request: Remove implicit inplace fix_precision for PointerTensor
Fixes #3261 

Problem statement:
fix_precision() is inplace when applied to a pointer tensor.
**Description of the bug**
The method fix_precision() applied on pointers is 'inplace'. This should not be the case

In the following code:

```
a = torch.Tensor([2., 3.]).send(bob)
a.fix_precision()
a = a.get()
print(type(a))
```

The variable  `a` after calling `get()` is a fixed precision tensor which is a bug, because the original variable `a` defined as `torch.Tensor([2., 3.])` is not a fixed precision tensor.

However, this bug is not existing in case when  `a` is not a pointer:

```
a = torch.Tensor([2., 3.])
a.fix_precision()
print(type(a))
```

`a` here is not a fixed precision tensor. which is the desired behavior.


**Desktop:**
 - OS: Archlinux
 - Version 0.3.2

Hints from issue discussion:
I can take this!!
Hey @AlanAboudib @karlhigley @LaRiffle ,
so if I understand this correctly,

```python
x = th.Tensor([1.,2.,3.]).send(bob)
x_fx = x.fix_precision()
x = x_fx.get()

```
x_fx is a `PointerTensor` and expected behavior is to be a `FixedPrecisionTensor` with `PointerTensor` as child?

Moreover when we .get() the x_fx, it returns `FixedPrecisionTensor`, which I think is a correct behavior, as we haven't called float_precision yet.
not exactly, fix_precision() should be applied anyway on the remote value when called on a pointer (and it works fine)

the issue is that currently we have this for pointers:
```
a = torch.Tensor([2., 3.]).send(bob)
a_fp = a.fix_precision()
# a_fp is a pointer to a fixed precision, but a is too!
```
while for non pointers:
```
a = torch.Tensor([2., 3.])
a_fp = a.fix_precision()
# a_fp is a wrapper onto a fixed precision, but a is not!
```

So for pointers, `fix_precision` behaves as `fix_precision_` while it shouldn't

Commit message: Add test

Patch to review:
diff --git a/syft/frameworks/torch/tensors/interpreters/native.py b/syft/frameworks/torch/tensors/interpreters/native.py
index d69acac7a5f..8a6ce38fc13 100644
--- a/syft/frameworks/torch/tensors/interpreters/native.py
+++ b/syft/frameworks/torch/tensors/interpreters/native.py
@@ -812,12 +812,14 @@ def fix_prec(self, *args, storage="auto", field_type="int100", no_wrap: bool = F
         if not kwargs.get("owner"):
             kwargs["owner"] = self.owner
 
+        # we create a copy of pointer which prevents inplace fix precision
         if self.is_wrapper:
-            self.child = self.child.fix_prec(*args, **kwargs)
+            self_copy = self.copy()
+            self_copy.child = self.child.fix_prec(*args, **kwargs)
             if no_wrap:
-                return self.child
+                return self_copy.child
             else:
-                return self
+                return self_copy
 
         base = kwargs.get("base", 10)
         prec_fractional = kwargs.get("precision_fractional", 3)
diff --git a/test/torch/pointers/test_pointer_tensor.py b/test/torch/pointers/test_pointer_tensor.py
index eeca0658441..f71e5cfe370 100644
--- a/test/torch/pointers/test_pointer_tensor.py
+++ b/test/torch/pointers/test_pointer_tensor.py
@@ -401,17 +401,23 @@ def test_raising_error_when_item_func_called(workers):
 def test_fix_prec_on_pointer_tensor(workers):
     """
     Ensure .fix_precision() works as expected.
+    Also check that fix_precision() is not inplace.
     """
     bob = workers["bob"]
 
     tensor = torch.tensor([1, 2, 3, 4.0])
     ptr = tensor.send(bob)
 
-    ptr = ptr.fix_precision()
+    ptr_fp = ptr.fix_precision()
+
     remote_tensor = bob._objects[ptr.id_at_location]
+    remote_fp_tensor = bob._objects[ptr_fp.id_at_location]
+
+    # check that fix_precision is not inplace
+    assert (remote_tensor == tensor).all()
 
     assert isinstance(ptr.child, PointerTensor)
-    assert isinstance(remote_tensor.child, FixedPrecisionTensor)
+    assert isinstance(remote_fp_tensor.child, FixedPrecisionTensor)
 
 
 def test_fix_prec_on_pointer_of_pointer(workers):


Code review comments:
diff_hunk: @@ -812,12 +812,14 @@ def fix_prec(self, *args, storage="auto", field_type="int100", no_wrap: bool = F
         if not kwargs.get("owner"):
             kwargs["owner"] = self.owner
 
+        # we create a copy of pointer which prevents inplace fix precision
         if self.is_wrapper:
-            self.child = self.child.fix_prec(*args, **kwargs)
+            self_copy = self.copy()
+            self_copy.child = self.child.fix_prec(*args, **kwargs)
path: syft/frameworks/torch/tensors/interpreters/native.py
text: @user1:
Actually I'm not 100% comfortable with copying the  tensor.
What about this?
```python
if self.is_wrapper:
    child = self.child.fix_prec(*args, **kwargs)
    if no_wrap:
        return child
    else:
        return child.wrap()
```
If this works, then I guess we should go in that direction:)

@author:
It works and this looks better approach!!