Features/227 lshape #231

ClaudiaComito · 2019-04-11T13:35:51Z

@Markus-Goetz there's still a quirk here with argmin and argmax. Even if axis = 0 (or contains 0), we can't lose the first dimension of partial, because partial_op in this case yields both the values and the indices, so dimension 0 of partial is by default at least 2 when it gets returned to the function. The fix has to happen in the argmin()/ argmax() functions. Not sure how to do that (yet).

Example:

>>> a = ht.random.randn(3,4,5)
>>> a
tensor([[[-0.3470, -0.3984,  0.0270,  0.3000,  1.0776],
         [-1.2756, -0.8707,  0.8296, -0.7750, -1.5317],
         [-0.2380, -0.3360, -1.4865,  0.5521, -0.2564],
         [-0.6830, -0.3591, -1.3159,  0.5883,  1.9217]],

        [[ 0.4855, -1.9448,  0.6220,  1.9253, -0.8722],
         [ 0.3382, -0.5159,  0.5053,  1.6421, -1.1279],
         [-0.5231, -0.1203,  1.3909,  0.4133,  0.6540],
         [-1.3037, -1.0900, -0.3871, -0.7963,  0.9564]],

        [[-1.0519,  0.0965, -0.9878, -0.0702, -0.2331],
         [ 0.0111, -1.1165, -1.6232, -0.1156, -0.3195],
         [-0.1528,  0.5501,  0.2335,  0.1948,  0.7384],
         [-1.8235,  1.4258,  0.2019,  1.0923,  1.0515]]])
>>> a.argmin(0)
tensor([[[2, 1, 2, 2, 1],
         [0, 2, 2, 0, 0],
         [1, 0, 0, 2, 0],
         [2, 1, 0, 1, 1]]])
>>> a.argmin(0).shape
(4, 5)
>>> a.argmin(0).lshape
(1, 4, 5)

With axis = 1:

>>> a.argmin(1)
tensor([[1, 1, 2, 1, 1],
        [3, 0, 3, 3, 1],
        [3, 1, 1, 1, 1]])
>>> a.argmin(1).shape
(3, 5)
>>> a.argmin(1).lshape
(3, 5)

… default.

Markus-Goetz · 2019-04-16T08:12:55Z

Could the fix be a squeeze() after the partial call?

ClaudiaComito · 2019-04-24T07:49:45Z

Could the fix be a squeeze() after the partial call?

Thanks, indeed, that's probably what I need. I'll work on it now.

codecov-io · 2019-04-24T07:50:59Z

Codecov Report

Merging #231 into master will increase coverage by 0.01%.
The diff coverage is 96.89%.

@@            Coverage Diff             @@
##           master     #231      +/-   ##
==========================================
+ Coverage   96.17%   96.18%   +0.01%     
==========================================
  Files          47       47              
  Lines        6514     6639     +125     
==========================================
+ Hits         6265     6386     +121     
- Misses        249      253       +4

Impacted Files	Coverage Δ
heat/core/tests/test_arithmetics.py	`100% <ø> (ø)`	⬆️
heat/core/tests/test_factories.py	`100% <100%> (ø)`	⬆️
heat/core/operations.py	`91.36% <100%> (+0.59%)`	⬆️
heat/core/__init__.py	`100% <100%> (ø)`	⬆️
heat/core/tests/test_rounding.py	`100% <100%> (ø)`	⬆️
heat/core/tests/test_exponential.py	`100% <100%> (ø)`	⬆️
heat/core/tests/test_statistics.py	`100% <100%> (ø)`	⬆️
heat/core/logical.py	`95% <100%> (ø)`	⬆️
heat/core/tests/test_manipulations.py	`100% <100%> (ø)`
heat/core/tests/test_logical.py	`100% <100%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ecac2d...59f2cc2. Read the comment docs.

…tests.

coquelin77 · 2019-04-29T06:49:28Z

heat/core/operations.py

@@ -208,8 +209,7 @@ def allclose(x, y, rtol=1e-05, atol=1e-08, equal_nan=False):
    return bool(_local_allclose.item())


-
-def any(x, axis=None, out=None):
+def any(x, axis=None, out=None, keepdim=None):


should this be keepdim=False instead of None?

Done (now in logical.py).

coquelin77 · 2019-04-29T06:53:19Z

heat/core/operations.py

+    x : ht.tensor
+        Input data.
+
+    axis : None or int or tuple of ints, optional


can you add a bit of context to what this axis parameter does? i can see it in the examples but some text would be nice too

Done (now in manipulations.py)

coquelin77 · 2019-04-29T07:01:41Z

heat/core/operations.py

+                if 0 in axis:
+                    lshape_losedim = (2,) + lshape_losedim
+                else:
+                    lshape_losedim = (2 * lshape_losedim[0],) + lshape_losedim[1:]


the 2 seems fishy to me. can you justify it for me? I am just not seeing it

Hi Daniel @coquelin77, EDITED
thanks, I'm really unhappy with this if statement. The problem is that, if axis is not None, the result of local_argmin/max and MPI_ARGMIN/MAX is made up of two sets of results really: the min/max values along a dimension, and the respective indices. So at this stage the dimension along the FIRST axis is the reduced dimension * 2. The min/max values are removed from the result at a later stage in the argmin/max() functions.

EDITED: never mind the par. below, I'm trying something else now.

(So having to re-add dimension of size 2 for the reduction axis is justified, it's cumbersome though and this "special behaviour" of argmin/max is giving me a lot of trouble pretty much whatever I'm working on. So @Markus-Goetz, @coquelin77 I'm thinking of adding __argreduce_op to operations.py and funnel argmin and argmax out to that function, leaving __reduce_op for "regular" reduction operations. )

OK here's what it looks like now (operations.py lines 217-221)

# Take care of special cases argmin and argmax: keep partial.shape[0] if (0 in axis and partial.shape[0] != 1): lshape_losedim = (partial.shape[0],) + lshape_losedim if (not 0 in axis and partial.shape[0] != x.lshape[0]): lshape_losedim = (partial.shape[0],) + lshape_losedim[1:]

Basically, the assumption is that whenever the first dimension of partial is different from what it should be (1 if reduction along axis 0, or x.lshape[0] if reduction along any other axis), there will be a good reason for it so just keep that partial[0] as first dimension.

This way we don't need to add ifs for every quirky reduction operation that comes our way.

Please let me know if I'm overseeing something.

coquelin77 · 2019-04-29T07:02:21Z

heat/core/tensor.py

@@ -278,7 +280,7 @@ def allclose(self, other, rtol=1e-05, atol=1e-08, equal_nan=False):
        """
        return operations.allclose(self, other, rtol, atol, equal_nan)

-    def any(self, axis=None, out=None):
+    def any(self, axis=None, out=None, keepdim=None):


keepdim=False vs =None again

done (dndarray.py)

coquelin77 · 2019-04-29T07:02:50Z

heat/core/tensor.py

+        x : ht.tensor
+            Input data.
+
+        axis : None or int or tuple of ints, optional


more explanation for axis as mentioned before

done (dndarray.py)

Replaced dodgy if statement with a more general formulation.

(Re-implementation after merging with master)

…lations.squeeze()

…ape is (1,).

… to any().

Markus-Goetz · 2019-05-14T07:17:03Z

Bump, has a minor conflict with the master

…r up to 4 ranks.

until final implementation of Allgatherv is available

ClaudiaComito · 2019-05-23T12:50:45Z

Alright @Markus-Goetz , @coquelin77 , I've commented out the test that fails for now, fixing that (Issue #273) needs fixing Allgatherv (#233, @Cdebus ). It would still be good to have squeeze() and the lshape fixes merged into master.

Thanks,

Claudia

ClaudiaComito added 5 commits April 11, 2019 12:41

Fixed operations.__reduce_op, lshape now losing reduced dimensions by…

471d4b9

… default.

Merge branch 'master' into features/227-lshape

26f1561

Added explicit keyword argument keepdim to any().

960a801

Fixed lshape for special cases argmin() and argmax()

8353893

Fixed tests.

7048b2b

ClaudiaComito requested a review from Markus-Goetz April 11, 2019 13:35

Merge branch 'master' into features/227-lshape

df5a2f6

ClaudiaComito added 8 commits April 24, 2019 13:29

Implementation of ht.squeeze(), first pass (local)

31b0de1

Implemented method squeeze() for ht.tensor.

7224b04

Broken. Added test_operations.test_squeeze()

060d6a5

Broken. Implemented squeeze() if operation is distributed.

7cccb14

Fixed. Raising exception if axis == split in distributed mode. Fixed …

8a5cccd

…tests.

Fixed documentation. Solves Issue #236

9467bce

Resolved lshape/gshape mismatch for argmin/argmax(axis=0).

d4291da

Fixed unwanted 0-dimension in output buffer.

d9de305

ClaudiaComito requested review from coquelin77, Cdebus and krajsek April 25, 2019 14:08

Merge branch 'master' into features/227-lshape

2d2a91a

coquelin77 reviewed Apr 29, 2019

View reviewed changes

ClaudiaComito added 7 commits May 7, 2019 14:49

(Broken) Merge branch 'master' into features/227-lshape

751fd50

(Broken) Introducing manipulations.py, test_manipulations.py

308db78

Addressing shape of partial in argmin/argmax case.

0ef3a69

Replaced dodgy if statement with a more general formulation.

Resolved lshape/gshape mismatch for argmin/argmax(axis=0).

f160e45

(Re-implementation after merging with master)

Added manipulations to __init__.py, squeeze() method now calls manipu…

50ee203

…lations.squeeze()

Merge branch 'master' into features/227-lshape

4d594de

Removed file test_relations.py to match latest master.

9f5c35f

ClaudiaComito added 9 commits May 9, 2019 10:14

__reduce_op to keep dimension in spite of keepdim=False if partial.sh…

e3b5062

…ape is (1,).

Importing factories module, modified tensor.array to factories.array.

696e60d

ht.squeeze(): adjust split axis of result according to squeezed shape

e86a6ea

Fixed keepdim mismatch in test_statistics.py

a1fb9f5

argmin(): Output buffer need not go through __reduce_op().

5d2ab31

Adapted tests to lshape being same as gshape.

cba72d2

Fixed tests.

a10ffdb

Added axis explanation to squeeze() docs, added default keepdim=False…

168dfe0

… to any().

Merge branch 'master' into features/227-lshape

139b627

ClaudiaComito mentioned this pull request May 10, 2019

operations_reduce__op(): keepdim=False must be default for local operations as well #227

Closed

ClaudiaComito added 11 commits May 15, 2019 08:45

Merge branch 'master' into features/227-lshape

5fda579

Merge branch 'master' into features/227-lshape

cb60791

Merged manipulation.py into manipulations.py incl. unit tests.

f882965

Removing calls to "manipulation" or replacing them with "manipulations".

a726c62

Merge branch 'master' into features/227-lshape

6709144

Fixed unit tests.

eec8cf3

Merge branch 'master' into features/227-lshape

5180e71

test_squeeze() workaround: enforce even distribution of dimensions fo…

0719375

…r up to 4 ranks.

Removed annoying line break

50b92a5

Merge branch 'master' into features/227-lshape

c66c8da

Commented out test_squeeze() "4D split tensor, along the axis"

7044062

until final implementation of Allgatherv is available

ClaudiaComito mentioned this pull request May 23, 2019

Reinstate portion of test_squeeze() testing uneven tensor distribution across ranks #273

Closed

Referenced relevant issues #273, #233

0ad2e5f

ClaudiaComito added 2 commits May 27, 2019 10:57

Merge branch 'master' into features/227-lshape

38396fb

Mentioning known issues #273, #233 that will be addressed separately.

59f2cc2

ClaudiaComito merged commit 5955563 into master May 27, 2019

ClaudiaComito deleted the features/227-lshape branch May 27, 2019 09:17

ClaudiaComito mentioned this pull request May 31, 2019

Subsequent reduction operations yield wrong result #228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/227 lshape #231

Features/227 lshape #231

ClaudiaComito commented Apr 11, 2019 •

edited

Loading

Markus-Goetz commented Apr 16, 2019

ClaudiaComito commented Apr 24, 2019 •

edited

Loading

codecov-io commented Apr 24, 2019 •

edited

Loading

coquelin77 Apr 29, 2019

ClaudiaComito May 9, 2019

coquelin77 Apr 29, 2019

ClaudiaComito May 9, 2019

coquelin77 Apr 29, 2019

ClaudiaComito May 8, 2019 •

edited

Loading

ClaudiaComito May 9, 2019

coquelin77 Apr 29, 2019

ClaudiaComito May 9, 2019

coquelin77 Apr 29, 2019

ClaudiaComito May 9, 2019

Markus-Goetz commented May 14, 2019

ClaudiaComito commented May 23, 2019

Features/227 lshape #231

Features/227 lshape #231

Conversation

ClaudiaComito commented Apr 11, 2019 • edited Loading

Markus-Goetz commented Apr 16, 2019

ClaudiaComito commented Apr 24, 2019 • edited Loading

codecov-io commented Apr 24, 2019 • edited Loading

Codecov Report

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 May 8, 2019 • edited Loading

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

Markus-Goetz commented May 14, 2019

ClaudiaComito commented May 23, 2019

ClaudiaComito commented Apr 11, 2019 •

edited

Loading

ClaudiaComito commented Apr 24, 2019 •

edited

Loading

codecov-io commented Apr 24, 2019 •

edited

Loading

ClaudiaComito May 8, 2019 •

edited

Loading