Features/880 binop ben bou #902

ben-bou · 2022-01-18T20:30:43Z

Description

Issue/s resolved: #880

Changes proposed:

Add None / newaxis indexing to getitem
Fix the ht.equal Method. Previously it used the binop interface, which was wrong because it isn't a binop
Make the stack function compatible to not balanced arrays
check lshape-map before binop instead of after try...except because not all processes necessarily fail so synchronization is necessary anyway
redistribute OUT-OF-PLACE - binops should not alter their arguments
add support for unbalanced array when the other one is not split
check arrays for having the same split axis AFTER (shape)broadcasting added empty dimensions (?)
restructure input sanitation; reduce nested ifs

Type of change

New feature (breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
--->

Performance

Checks lshape-maps inside every binop between distributed dndarrays -> more communication
Redistributes out-of-place -> more memory

If the dndarrays are balanced (i.e. the features of this PR are not used) the introduced overhead is small.
Binops between two balanced dndarrays of shape (30,100,100) with 24 MPI-processes spend >90% of the runtime in the respective torch functions:

         53713 function calls (53203 primitive calls) in 1.866 seconds

   Ordered by: internal time
   List reduced from 51 to 30 due to restriction <30>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
      100    0.378    0.004    0.378    0.004 {built-in method mul}
      100    0.375    0.004    0.375    0.004 {built-in method add}
      100    0.375    0.004    0.375    0.004 {built-in method true_divide}
      100    0.370    0.004    0.370    0.004 {built-in method sub}
      100    0.286    0.003    0.286    0.003 {built-in method eq}
      500    0.017    0.000    1.858    0.004 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/_operations.py:24(__binary_op)
      500    0.008    0.000    0.008    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/dndarray.py:63(__init__)
      500    0.007    0.000    0.009    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/stride_tricks.py:12(broadcast_shape)
      500    0.006    0.000    0.009    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/sanitation.py:31(sanitize_distribution)
 1000/500    0.005    0.000    0.007    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/types.py:889(result_type_rec)
     2500    0.005    0.000    0.010    0.000 /p/software/juwels/stages/2020/software/SciPy-Stack/2020-gcccoremkl-9.3.0-2020.2.254-Python-3.8.5/lib/python3.8/site-packages/numpy-1.19.1-py3.8-linux-x86_64.egg/numpy/core/numeric.py:1816(isscalar)
      100    0.004    0.000    1.866    0.019 profile-binop.py:10(test_function)
     8500    0.003    0.000    0.005    0.000 {built-in method builtins.isinstance}
     1500    0.003    0.000    0.004    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/types.py:495(canonical_heat_type)
     1000    0.002    0.000    0.002    0.000 {method 'type' of 'torch._C._TensorBase' objects}
      500    0.002    0.000    0.012    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/_operations.py:114(__get_out_params)
      500    0.002    0.000    0.005    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/types.py:565(heat_type_of)
     2500    0.001    0.000    0.001    0.000 {built-in method _abc._abc_instancecheck}
     1500    0.001    0.000    0.001    0.000 {built-in method builtins.issubclass}
     2000    0.001    0.000    0.001    0.000 {built-in method builtins.max}
     1000    0.001    0.000    0.001    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/dndarray.py:941(is_balanced)
      500    0.001    0.000    0.008    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/types.py:868(result_type)
     2500    0.001    0.000    0.002    0.000 /p/software/juwels/stages/2020/software/Python/3.8.5-GCCcore-9.3.0/lib/python3.8/abc.py:96(__instancecheck__)
      100    0.001    0.000    0.301    0.003 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/relational.py:35(eq)
     5000    0.001    0.000    0.001    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/dndarray.py:293(shape)
      500    0.001    0.000    0.001    0.000 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/dndarray.py:279(lshape_map)
     5500    0.001    0.000    0.001    0.000 {built-in method builtins.len}
      100    0.001    0.000    0.385    0.004 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/arithmetics.py:904(sub)
      100    0.001    0.000    0.390    0.004 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/arithmetics.py:63(add)
      100    0.001    0.000    0.390    0.004 /p/project/cslts/local/juwels/HeAT/binop_Branch/lib/python3.8/site-packages/heat/core/arithmetics.py:430(div)

Due Diligence

All split configurations tested
Multiple dtypes tested in relevant functions
Documentation updated (if needed)
Updated changelog.md under the title "Pending Additions"

Does this change modify the behaviour of other functions? If so, which?

yes, every function using binops,

…/binop_distribute

…shape_map attribute -> return copy of the lshape_map

…oltz-analytics/heat into features/880-binop_ben-bou

ClaudiaComito · 2022-01-25T08:48:06Z

run tests

…lytics/heat into features/880-binop_ben-bou

ClaudiaComito · 2022-01-25T09:22:23Z

run tests

ClaudiaComito · 2022-01-28T11:01:10Z

heat/core/dndarray.py

@@ -582,15 +582,15 @@ def create_lshape_map(self, force_check: bool = False) -> torch.Tensor:
            result. Otherwise, create the lshape_map
        """
        if not force_check and self.__lshape_map is not None:
-            return self.__lshape_map
+            return self.__lshape_map.clone()


I can see why you do it, but the lshape_map can get quite big with many nodes. Why not cloning or copying as need arises?

Well somehow it must be ensured that the attribute is immutable, and as far as I know, there are no read-only Tensors.

most of the time this shouldnt be a problem. the lshape_map is generally much smaller than the other tensor sizes

ClaudiaComito · 2022-01-28T11:01:21Z

heat/core/dndarray.py

@@ -601,7 +601,7 @@ def create_lshape_map(self, force_check: bool = False) -> torch.Tensor:
            self.comm.Allreduce(MPI.IN_PLACE, lshape_map, MPI.SUM)

        self.__lshape_map = lshape_map
-        return lshape_map
+        return lshape_map.clone()


ClaudiaComito · 2022-01-28T11:01:39Z

heat/core/dndarray.py

+        # for dim in expand:
+        #     self = self.expand_dims(dim)
+        # if len(expand):
+        #     return self[tuple(key)]


ClaudiaComito · 2022-01-28T11:01:48Z

heat/core/dndarray.py

-        key = tuple(key)
+        self_proxy = self.__torch_proxy__()
+        # None and newaxis indexing
+        # expand = []


ClaudiaComito · 2022-01-28T11:03:32Z

heat/core/manipulations.py

-        array_dtype = types.canonical_heat_type(t_array_dtype)
+    target = arrays[0]
+    try:
+        # arrays[1:] = sanitation.sanitize_distribution(*arrays[1:], target=target) # error in unpacking


Thanks. Another thing about this part: the try ... except is only needed to transform a NotImplementedError to a ValueError. At some point it would be good to have unified Errors within HeAT, such that e.g. a wrong split axis always yields the same Exception

ClaudiaComito · 2022-01-28T11:32:03Z

heat/core/_operations.py

+
+    def __get_out_params(target, other=None, map=None):
+        """
+        Getter for the output parameters of a binop with target.


"binop with target" -> "binary operation with target distribution"

ClaudiaComito · 2022-01-28T11:33:00Z

heat/core/_operations.py

+    def __get_out_params(target, other=None, map=None):
+        """
+        Getter for the output parameters of a binop with target.
+        If other is provided, it's distribution will be matched to target or, if provided,


other -> other
it's -> its
target -> target

ClaudiaComito · 2022-01-28T11:33:16Z

heat/core/_operations.py

+        """
+        Getter for the output parameters of a binop with target.
+        If other is provided, it's distribution will be matched to target or, if provided,
+        redistributed according to map.


map -> map

ClaudiaComito · 2022-01-28T11:34:52Z

heat/core/_operations.py

+        other : DNDarray
+            DNDarray to be adapted
+        map : Tensor
+            Lshape-Map other should be matched to. Defaults to target's lshape_map


Lshape-Map -> lshape_map
other -> other
target's lshape_map -> target.lshape_map

ClaudiaComito · 2022-01-28T11:35:36Z

heat/core/relational.py

+    # result_tensor = _operations.__binary_op(torch.equal, x, y)
+    #
+    # if result_tensor.larray.numel() == 1:
+    #     result_value = result_tensor.larray.item()
+    # else:
+    #     result_value = True
+    #
+    # return result_tensor.comm.allreduce(result_value, MPI.LAND)


ClaudiaComito · 2022-01-28T11:38:12Z

heat/core/sanitation.py

+) -> Union[DNDarray, Tuple(DNDarray)]:
+    """
+    Distribute every arg according to target.lshape_map or, if provided, diff_map.
+    After this sanitation, the lshapes are compatible along the split dimension.


arg, target.lshape_map, diff_map

ClaudiaComito

@ben-bou thanks again for so much work. The default cloning of the lshape_map is something we should rethink. Otherwise I've got mostly editorial changes.

coquelin77 · 2022-01-31T08:50:28Z

run tests

ClaudiaComito

Brillian @ben-bou , thank you so much!

ClaudiaComito and others added 30 commits November 2, 2021 13:52

Support different lshape maps in binary ops

346822a

Adapt documentation

169f062

Typos

207e843

Update changelog

cb053e6

Debugging tests

ff03641

Allow unbalanced and unequally balanced dndarrays for binops

1958470

determine promoted_type in the beginning

a5cc6ff

Refine distribution logic for non-distributed operands

86172ba

Change tests order

bd33252

Typo

e41dc58

typo

e1fb638

No broadcasting in split-dimension

871bb42

Merge branch 'features/880-bin_op_different_distribution' into origin…

55d46bf

…/binop_distribute

Remove redundant condition

c415539

explicit broadcasting in split-dimension

45dc427

Merge branch 'features/880-bin_op_different_distribution' into origin…

363f304

…/binop_distribute

debugging

2eaf0c5

improve tests

70ce2bc

fix empty process bug

2075684

remove debugging output

00c31bd

empty process handling; run tests

ed753a6

typo

c341248

move the equalization of distribution to sanitation

85d376e

beautify

50d9406

wrong condition

b6ba31b

use None-indexing, fix broadcast_shapes

6b3f136

Fix broadcast_shapes

572c432

old expand_dims

f5fa517

debug

c77d086

allow broadcasting in split dimension

7c7160d

Ben Bourgart and others added 5 commits January 24, 2022 18:53

changing Tensor returned by create_lshape_map should not change the l…

eac41b8

…shape_map attribute -> return copy of the lshape_map

Merge branch 'master' into features/880-binop_ben-bou

415ee52

add tests

e70aa39

Merge branch 'features/880-binop_ben-bou' of https://github.com/helmh…

ca23d4c

…oltz-analytics/heat into features/880-binop_ben-bou

add tests

f05d31f

ClaudiaComito added 2 commits January 25, 2022 10:14

Edit error messages for sanitize_out

664b27e

Merge branch 'features/880-binop_ben-bou' of github.com:helmholtz-ana…

c6a7d9e

…lytics/heat into features/880-binop_ben-bou

ClaudiaComito reviewed Jan 28, 2022

View reviewed changes

ClaudiaComito requested changes Jan 28, 2022

View reviewed changes

adressed Review; mainly docs

3ff5a41

ClaudiaComito approved these changes Jan 31, 2022

View reviewed changes

ClaudiaComito merged commit dbb8300 into master Jan 31, 2022

ClaudiaComito deleted the features/880-binop_ben-bou branch January 31, 2022 09:02

ClaudiaComito mentioned this pull request Feb 8, 2022

Address distributed non-ordered indexing #914

Open

This was referenced Apr 1, 2022

bug in __binary_op #294

Closed

Binary Operations with wrong Output Shape #649

Closed

ClaudiaComito mentioned this pull request Feb 13, 2023

Bug/789 pow binary op performance #793

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/880 binop ben bou #902

Features/880 binop ben bou #902

ben-bou commented Jan 18, 2022 •

edited

ClaudiaComito commented Jan 25, 2022

ClaudiaComito commented Jan 25, 2022

ClaudiaComito Jan 28, 2022

ben-bou Jan 28, 2022

coquelin77 Jan 31, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ben-bou Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito Jan 28, 2022

ClaudiaComito left a comment

coquelin77 commented Jan 31, 2022

ClaudiaComito left a comment

Features/880 binop ben bou #902

Features/880 binop ben bou #902

Conversation

ben-bou commented Jan 18, 2022 • edited

Description

Changes proposed:

Type of change

Performance

Due Diligence

Does this change modify the behaviour of other functions? If so, which?

ClaudiaComito commented Jan 25, 2022

ClaudiaComito commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClaudiaComito left a comment

Choose a reason for hiding this comment

coquelin77 commented Jan 31, 2022

ClaudiaComito left a comment

Choose a reason for hiding this comment

ben-bou commented Jan 18, 2022 •

edited