Repository: OpenMined/PySyft

Issue:
Implement Default split Functionality for Base Tensor Type.
<!-- Please make sure that you review this: https://github.com/OpenMined/Docs/blob/master/contributing/guidelines.md  -->
<!-- If you are looking to file a bug make sure to look at the .github/BUG_ISSUE_TEMPLATE.md -->


#### User story: 
As a Data Scientist using Syft's Base Tensor type, I want to leverage a default method for computing operations on a Tensor of arbitrary type. For this ticket to be complete `split` should return a new tensor. For a reference on the operation these perform check out [PyTorch's](http://pytorch.org/docs/master/torch.html#torch.split) documentation.

<!-- Provide a detailed explaination about the proposed feature, you can draw inspiration from something like this: 
https://github.com/OpenMined/PySyft/issues/227 or https://github.com/OpenMined/PySyft/issues/12 -->

#### Acceptance Criteria: 

<!-- Provide an outline af all the things that needs to be addressed in order to close this Issue,
be as descriptive as possible -->
  - [x] If the Base Tensor type's attribute "encrypted" is set to True, it should return a NotImplemented error.
- [ ] corresponding unit tests demonstrating the correct operation on the Base Tensor type implemented over int and float Tensors.
 - [ ] inline documentation as described over [here](https://github.com/OpenMined/PySyft/blob/85bc68e81a2f4bfc0f0bf6c4252b88d6d7b54004/syft/math.py#L5). 


<!-- Thanks for your contributions! -->


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=h1) Report
> Merging [#391](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=desc) into [master](https://codecov.io/gh/OpenMined/PySyft/commit/f1f230c45e29abce37f83a2ed4606c8a6512d3fa?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/OpenMined/PySyft/pull/391/graphs/tree.svg?height=150&width=650&src=pr&token=W0kQS1vaXB)](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
+ Coverage   99.52%   99.52%   +<.01%     
==========================================
  Files           4        4              
  Lines        1682     1695      +13     
==========================================
+ Hits         1674     1687      +13     
  Misses          8        8
```


| [Impacted Files](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [PySyft/tests/test\_tensor.py](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=tree#diff-UHlTeWZ0L3Rlc3RzL3Rlc3RfdGVuc29yLnB5) | `99.35% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=footer). Last update [f1f230c...f714e87](https://codecov.io/gh/OpenMined/PySyft/pull/391?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

bharathgs: looks like few test are missing.
divyanshj16: Earlier I confused `split_size` variable to be the number of splits. But In the new commit, I changed this!
Actually, split size is chunk size.
If there is an array [0,1,2,3,4,5,6,7,8,9] then if `split_size` is 3 then the array will get divided into subarrays of size 3 and last one with size one as [[0,1,2],[3,4,5],[6,7,8],[9]].
In my earlier implementation the above array would have divided into 3 subarrays [[0,1,2,3],[4,5,6,7],[8,9]]. This has been resolved by this last commit.
bharathgs: your code does not produce the results that you mention in the comment above.
also, please try out the code before a PR.
divyanshj16: I'll do the changes! I did not understand your second point, `2. in place version of this function is not necessary, however, a split method is(in tensor module)`
bharathgs: change `split_` -> `split`
divyanshj16: There is already a `split` method in `tensor.py` at line 3041 and `SplitTests` are also there in `test_tensor.py` at line 977. Its tests are not exhaustive. Also, they are not implemented as PyTorch's implementation. 
This was the reason I had to name my test class as `SplitTests2` and my function as `split_`. 
My functions are working good in accordance with the PyTorch's implementation. See the image below.

![image](https://user-images.githubusercontent.com/16683472/32266444-76c3dc2e-bf0d-11e7-92f1-2dd50b7fd32f.png)

I'll add tests in test_math for the split function and will create a PR!
Should I  remove the earlier implementation(i.e implementation on the specified lines)?
bharathgs: ahh, now I see, I missed that. yes remove the earlier implementation
divyanshj16: Naming conventions are not specific as `axis`(numpy's) and `dim`(pytorch's) are interchangeably used in the whole code. What should I use?
divyanshj16: I have used `axis` for naming the axis of the tensor. However, we should raise an issue to make it consistent.

Patch:
diff --git a/syft/math.py b/syft/math.py
index e880c85ed9..4d395a6a0d 100644
--- a/syft/math.py
+++ b/syft/math.py
@@ -968,3 +968,43 @@ def zeros(dim):
         Output Tensor
     """
     return TensorBase(np.zeros(dim))
+
+
+def split(tensor, split_size, axis=0):
+    """
+    Splits the tensor into multiple equally sized chunks (if possible).
+
+    Last chunk will be smaller if the tensor size along a given axis
+    is not divisible by `split_size`.
+
+    Returns a list of the split tensors
+
+    Parameters
+    ----------
+    tensor: TensorBase
+        array to be divided into sub-arrays.
+
+    split_size: int
+        size of single chunk
+
+    axis: int, optional
+        The axis along which to split, default is 0.
+
+    Returns
+    -------
+    list: list of divided arrays
+    """
+    if axis < 0:
+        axis += len(tensor.shape())
+
+    length_along_axis = tensor.shape()[axis]
+
+    # calculate number of splits!
+    num_splits = (length_along_axis + split_size - 1) // split_size
+
+    # make array to pass to numpy array_split function
+    split_according = [split_size * i for i in range(1, num_splits)]
+
+    list_ = np.array_split(tensor.data, split_according, axis)
+
+    return list(map(lambda x: TensorBase(x), list_))
diff --git a/syft/tensor.py b/syft/tensor.py
index f774b294e1..89f23c3b05 100644
--- a/syft/tensor.py
+++ b/syft/tensor.py
@@ -3038,29 +3038,6 @@ class TensorBase(object):
         else:
             return self.data.shape
 
-    def split(self, split_size, dim=0):
-        """
-        Returns tuple of tensors of equally sized tensor/chunks (if possible)
-
-        Parameters
-        ----------
-        split_size:
-
-        dim:
-
-        Returns
-        -------
-        Tuple of Tensors
-        """
-        if self.encrypted:
-            return NotImplemented
-        splits = np.array_split(self.data, split_size, axis=0)
-        tensors = list()
-        for s in splits:
-            tensors.append(TensorBase(s))
-        tensors_tuple = tuple(tensors)
-        return tensors_tuple
-
     def stride(self, dim=None):
         """
         Returns the jump necessary to go from one element to the next one in the specified dimension dim.
@@ -3601,3 +3578,30 @@ class TensorBase(object):
 
         self.data.fill(0)
         return self
+
+    def split(self, split_size, axis=0):
+        """
+        Splits the tensor into multiple equally sized chunks (if possible).
+
+        Last chunk will be smaller if the tensor size along a given axis
+        is not divisible by `split_size`.
+
+        Returns a list of the split tensors
+
+        Parameters
+        ----------
+
+        split_size: int
+            size of single chunk
+
+        axis: int, optional
+            The axis along which to split, default is 0.
+
+        Returns
+        -------
+        list: list of divided arrays
+        """
+        if self.encrypted:
+            return NotImplemented
+
+        return syft.math.split(self, split_size, axis)


Test patch:
diff --git a/tests/test_math.py b/tests/test_math.py
index 83b590f47b..09aad0a9f7 100644
--- a/tests/test_math.py
+++ b/tests/test_math.py
@@ -427,3 +427,17 @@ class MultinomialTests(unittest.TestCase):
         t2 = syft.math.multinomial(t1, len(t1))
         self.assertTupleEqual((len(t1),), t2.shape())
         self.assertTrue(np.all(t2.data >= 0) and np.all(t2.data <= len(t1)))
+
+
+class SplitTests(unittest.TestCase):
+    def test_split(self):
+        t = TensorBase(np.random.rand(7, 4))
+        split_size = 3
+        axis = 0
+        target_shapes = [(3, 4), (3, 4), (1, 4)]
+        splits = syft.math.split(t, split_size, axis)
+        start = 0
+        for target_shape, split in zip(target_shapes, splits):
+            self.assertTrue(syft.equal(split.shape(), target_shape))
+            self.assertTrue(syft.equal(t.narrow(axis, start, target_shape[axis]), split))
+            start += target_shape[axis]
diff --git a/tests/test_tensor.py b/tests/test_tensor.py
index c34b4cebc6..3c9052c3bf 100644
--- a/tests/test_tensor.py
+++ b/tests/test_tensor.py
@@ -974,13 +974,6 @@ class CumprodTest(unittest.TestCase):
         self.assertTrue(np.equal(t1.cumprod_(dim=1), t3).all())
 
 
-class SplitTests(unittest.TestCase):
-    def test_split(self):
-        t1 = TensorBase(np.arange(8.0))
-        t2 = t1.split(4)
-        self.assertTrue(np.array_equal(t2, tuple((np.array([0., 1.]), np.array([2., 3.]), np.array([4., 5.]), np.array([6., 7.])))))
-
-
 class SqueezeTests(unittest.TestCase):
     def test_squeeze(self):
         t1 = TensorBase(np.zeros((2, 1, 2, 1, 2)))
@@ -1628,6 +1621,20 @@ class UnfoldTest(unittest.TestCase):
                                        t1_unfolded_actual_2))
 
 
+class SplitTests(unittest.TestCase):
+    def test_split(self):
+        t = TensorBase(np.random.rand(10, 5))
+        split_size = 3
+        axis = 0
+        target_shapes = [(3, 5), (3, 5), (3, 5), (1, 5)]
+        splits = t.split(split_size, axis)
+        start = 0
+        for target_shape, split in zip(target_shapes, splits):
+            self.assertTrue(syft.equal(split.shape(), target_shape))
+            self.assertTrue(syft.equal(t.narrow(axis, start, target_shape[axis]), split))
+            start += target_shape[axis]
+
+
 if __name__ == "__main__":
 
     unittest.main()