Bug fix / Enhancement array initialization different devices #678

mtar · 2020-09-25T14:27:11Z

Description

Refines the same device check on array initialization and also fixes the calling functions that it will not be called any longer. Kept as an assertion instead.

Lowers coverage

Issue/s resolved: #675

Changes proposed:

Refine the device check in DNDarray.__init__ and change it into an assertion
get right device in factories.array
- remove the device id in devices.cpu.torch_device to be akin to str(torch.device)
- add "cuda" in devices.__device_mapping allowing to pass torch.device.type to devices.sanitize.device
various changes on functions creating instances of DNDarray directly

Type of change

Bug fix

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?

no

codecov · 2020-09-25T14:34:54Z

Codecov Report

Merging #678 into release/0.5.x will decrease coverage by 0.02%.
The diff coverage is 92.30%.

@@                Coverage Diff                @@
##           release/0.5.x     #678      +/-   ##
=================================================
- Coverage          97.46%   97.43%   -0.03%     
=================================================
  Files                 87       87              
  Lines              17101    17110       +9     
=================================================
+ Hits               16667    16671       +4     
- Misses               434      439       +5

Impacted Files	Coverage Δ
heat/core/devices.py	`85.00% <50.00%> (-2.18%)`	⬇️
heat/core/factories.py	`99.05% <77.77%> (-0.95%)`	⬇️
heat/core/_operations.py	`93.61% <100.00%> (ø)`
heat/core/dndarray.py	`96.43% <100.00%> (-0.28%)`	⬇️
heat/core/io.py	`90.32% <100.00%> (ø)`
heat/core/logical.py	`100.00% <100.00%> (ø)`
heat/core/manipulations.py	`99.22% <100.00%> (ø)`
heat/core/random.py	`100.00% <100.00%> (ø)`
heat/core/tests/test_exponential.py	`100.00% <100.00%> (ø)`
heat/core/tests/test_manipulations.py	`99.92% <100.00%> (ø)`
... and 3 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 88f9157...27d7134. Read the comment docs.

Bugfix remove implicit tests when wrong device is given to DNDarray.

ClaudiaComito

Well done, thanks a lot for this. Was this all the device inconsistencies? I'm surprised we haven't got more.

I only have a few comments.

heat/core/factories.py

ClaudiaComito · 2020-10-05T08:52:00Z

heat/core/factories.py

+
+    # change device if it do not match
+    if str(obj.device) != device.torch_device:
+        obj = obj.to(device.torch_device)


why is the implicit data transfer ok here? 🤔

It's analogous to dtype in factories.array(). The user explicitly wants to use a specific device here.

On internal functions, however, it still can obfuscate some wrong devices if the device parameter is overused. I add a warning.

OK, yes I can see why we want to transfer data here. On internal functions, we have to keep an eye on it during review and testing I guess.

ClaudiaComito · 2020-10-05T09:08:22Z

CHANGELOG.md

@@ -1,3 +1,7 @@
+# v0.5.1
+
+- [#678](https://github.com/helmholtz-analytics/heat/pull/678) Bugfix: Many implicit array transfers on DNDarray initialization


Can you be more descriptive? If I get it right, we are avoiding implicit array transfers or replacing them with ...

(Apologies for this comment, having spent so much time trying to figure out what our Pending additions for 0.5.0 meant, I'm a bit traumatized 😅 )

Possible change for clarity

Bugfix: Internal functions now use explicit device parametes for DNDarray and torch.Tensor initializations.

ClaudiaComito

I'm fine with this, thanks Michael!

mtar · 2020-10-07T05:55:22Z

I'm fine with this, thanks Michael!

But I'm not satisfied anymore 😃
After adding a warning, I found out that TestGaussianNB::test_fit_iris is causing a device transfer.

mtar · 2020-10-08T10:40:24Z

run tests

mtar · 2020-10-09T09:46:51Z

heat/core/factories.py

+    assert str(obj.device) == device.torch_device
+
+    if str(obj.device) != device.torch_device:
+        warnings.warn(
+            "Array 'obj' is not on device '{}'. It will be copied to it.".format(device),
+            UserWarning,
+        )


Should I keep the assertion or the warning here?

I deleted the assertion as there are some rare cases where it is allowed. However the unittest results must be reviewed for possible wrong devices.

coquelin77 · 2020-10-26T07:44:49Z

CHANGELOG.md

@@ -1,3 +1,7 @@
+# v0.5.1
+
+- [#678](https://github.com/helmholtz-analytics/heat/pull/678) Bugfix: Many implicit array transfers on DNDarray initialization


Possible change for clarity

Bugfix: Internal functions now use explicit device parametes for DNDarray and torch.Tensor initializations.

add missing device changes for gpu

9566f45

mtar changed the title ~~add missing device changes for gpu~~ Add missing device parameters Sep 25, 2020

mtar changed the base branch from master to release/0.5.x September 28, 2020 10:11

mtar added 2 commits September 28, 2020 12:16

Merge branch 'release/0.5.x' into bug/675-initdevice

7968876

Bugfix remove implicit tests when wrong device is given to DNDarray.

update changelog

e4e2d2a

mtar marked this pull request as ready for review September 28, 2020 11:58

mtar changed the title ~~Add missing device parameters~~ Bug fix / Enhancement array initialization different devices Sep 28, 2020

mtar requested review from Markus-Goetz, coquelin77 and ClaudiaComito September 28, 2020 12:00

rework old code

82b523f

ClaudiaComito requested changes Oct 5, 2020

View reviewed changes

mtar added 2 commits October 5, 2020 16:07

Add warning on different devices

b93bccb

change to UserWarning

7b8fb1a

ClaudiaComito approved these changes Oct 7, 2020

View reviewed changes

mtar added 11 commits October 7, 2020 08:50

change device in test_gaussian

133def1

test device

cd606d8

test device

b0dc5d6

skip exceptions

159b2b5

before first fit

7eff3a5

use assertion

0e60691

revert changes testgaussian

33551b8

str missing

6637fa2

load_csv device

b835d1e

device in unique

7ce55e7

change device directly in test_exp2

bd5903c

change device earlier in assert_func_equal_tensor

c3ab419

mtar added 4 commits October 8, 2020 13:33

add device in test_assert_func_equal_for_tensor

a52c92b

convert to cpu before numpy

62e1e6b

add torch device is_close

c8b4d1e

formatting issues

889f2be

mtar commented Oct 9, 2020

View reviewed changes

mtar added 3 commits October 13, 2020 12:36

Merge branch 'release/0.5.x' into bug/675-initdevice

c1705bc

use warning with stacklevel 2

09290f9

formatting

ca5f688

coquelin77 approved these changes Oct 26, 2020

View reviewed changes

mtar added 2 commits October 26, 2020 10:21

update changelog

6046ab7

decrease stacklevel of warning

27d7134

mtar merged commit 9c90cb4 into release/0.5.x Oct 26, 2020

mtar deleted the bug/675-initdevice branch October 26, 2020 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fix / Enhancement array initialization different devices #678

Bug fix / Enhancement array initialization different devices #678

mtar commented Sep 25, 2020 •

edited

Loading

codecov bot commented Sep 25, 2020 •

edited

Loading

ClaudiaComito left a comment

ClaudiaComito Oct 5, 2020

mtar Oct 5, 2020

mtar Oct 5, 2020 •

edited

Loading

ClaudiaComito Oct 7, 2020

ClaudiaComito Oct 5, 2020

coquelin77 Oct 26, 2020

ClaudiaComito left a comment

mtar commented Oct 7, 2020

mtar commented Oct 8, 2020

mtar Oct 9, 2020

mtar Oct 14, 2020 •

edited

Loading

coquelin77 Oct 26, 2020

		@@ -1,3 +1,7 @@
		# v0.5.1

		- [#678](https://github.com/helmholtz-analytics/heat/pull/678) Bugfix: Many implicit array transfers on DNDarray initialization

Bug fix / Enhancement array initialization different devices #678

Bug fix / Enhancement array initialization different devices #678

Conversation

mtar commented Sep 25, 2020 • edited Loading

Description

Changes proposed:

Type of change

Due Diligence

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

codecov bot commented Sep 25, 2020 • edited Loading

Codecov Report

ClaudiaComito left a comment

Choose a reason for hiding this comment

ClaudiaComito Oct 5, 2020

Choose a reason for hiding this comment

mtar Oct 5, 2020

Choose a reason for hiding this comment

mtar Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

ClaudiaComito Oct 7, 2020

Choose a reason for hiding this comment

ClaudiaComito Oct 5, 2020

Choose a reason for hiding this comment

coquelin77 Oct 26, 2020

Choose a reason for hiding this comment

ClaudiaComito left a comment

Choose a reason for hiding this comment

mtar commented Oct 7, 2020

mtar commented Oct 8, 2020

mtar Oct 9, 2020

Choose a reason for hiding this comment

mtar Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

coquelin77 Oct 26, 2020

Choose a reason for hiding this comment

mtar commented Sep 25, 2020 •

edited

Loading

codecov bot commented Sep 25, 2020 •

edited

Loading

mtar Oct 5, 2020 •

edited

Loading

mtar Oct 14, 2020 •

edited

Loading