-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug/825 setitem slice dndarrays #826
Conversation
… in the split dimension, lshape_map property added
Codecov Report
@@ Coverage Diff @@
## master #826 +/- ##
==========================================
+ Coverage 95.30% 95.32% +0.02%
==========================================
Files 64 64
Lines 9004 9056 +52
==========================================
+ Hits 8581 8633 +52
Misses 423 423
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it up! @coquelin77
I played around a little bit. It doesn't work when the split axes are different. Do you want to handle them too?
Some notes I took when reviewing the code. You wanted to clean it up anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the new lshape_map
property. Brilliant.
A problem I'm having with setitem
is this:
>>> import heat as ht
>>> a = ht.arange(10).reshape(2,5)
>>> a[1,ht.array([1])]
DNDarray([6], dtype=ht.int32, device=cpu:0, split=None)
>>> a[1,ht.array(1)]
DNDarray(6, dtype=ht.int32, device=cpu:0, split=None)
a[1,ht.array([1])] = 7
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/c.comito/HAF/heat/heat/core/dndarray.py", line 1404, in __setitem__
return self.__setter(key, value) # returns None
File "/Users/c.comito/HAF/heat/heat/core/dndarray.py", line 1570, in __setter
self.__array.__setitem__(key, value)
File "/Users/c.comito/HAF/heat/heat/core/dndarray.py", line 976, in __len__
return self.shape[0]
This is not specific to this branch but maybe can be addressed here.
Thanks a lot!
do you mean to the if you mean the |
From the example in #825: x = ht.ones((10, 10), split=0)
setting = ht.zeros((8, 8), split=1)
x[1:-1, 1:-1] = setting
print(x) This doesn't work. This works however x = ht.ones((10, 10), split=0)
setting = ht.zeros((4, 8), split=1)
x[0:4, 1:-1] = setting
print(x) [1,0]<stdout>:DNDarray([[1., 0., 0., 0., 0., 0., 0., 0., 0., 1.],
[1,0]<stdout>: [1., 0., 0., 0., 0., 0., 0., 0., 0., 1.],
[1,0]<stdout>: [1., 0., 0., 0., 0., 0., 0., 0., 0., 1.],
[1,0]<stdout>: [1., 0., 0., 0., 0., 0., 0., 0., 0., 1.],
[1,0]<stdout>: [1., 1., 1., 1., 1., 1., 1., 1., 1., 1.],
[1,0]<stdout>: [1., 1., 1., 1., 1., 1., 1., 1., 1., 1.],
[1,0]<stdout>: [1., 1., 1., 1., 1., 1., 1., 1., 1., 1.],
[1,0]<stdout>: [1., 1., 1., 1., 1., 1., 1., 1., 1., 1.],
[1,0]<stdout>: [1., 1., 1., 1., 1., 1., 1., 1., 1., 1.],
[1,0]<stdout>: [1., 1., 1., 1., 1., 1., 1., 1., 1., 1.]], dtype=ht.float32, device=cpu:0, split=0)
[1,1]<stdout>: |
with some digging. the example that works in this case, only works because of an accident where all of the values are gathered onto rank 0. I think that setting a split DNDarray with another differently split DNDarray should be handled in another issue |
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you increase the coverage?
array indexing is not available a[ht.array([0,1])] = ht.ones(2)
, but this is another issue.
CHANGELOG.md
Outdated
@@ -50,6 +50,7 @@ Example on 2 processes: | |||
- [#820](https://github.com/helmholtz-analytics/heat/pull/820) `randn` values are pushed away from 0 by the minimum value the given dtype before being transformed into the Gaussian shape | |||
- [#821](https://github.com/helmholtz-analytics/heat/pull/821) Fixed `__getitem__` handling of distributed `DNDarray` key element | |||
- [#826](https://github.com/helmholtz-analytics/heat/pull/826) Fixed `__setitem__` handling of distributed `DNDarray` values which have a different shape in the split dimension | |||
- [#831](https://github.com/helmholtz-analytics/heat/pull/831) `__getitem__` handling of `array-like` 1-element key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending Additions
f"\nvalue.split {val_split} not equal to this DNDarray's split:" | ||
f" {sp}. this may cause errors or unwanted behavior", | ||
category=RuntimeWarning, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😘
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a test for the warning? No test according to codecov
Co-authored-by: mtar <m.tarnawa@fz-juelich.de>
…helmholtz-analytics/heat into bug/825-setitem-slice-dndarrays
step2 = key_step if key_step is not None else 1 | ||
key_start = (chunk_starts[rank] - og_key_start).item() | ||
if key_start < 0: | ||
key_start = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no test according to codecov
Description
Fix for setting DNDarray values with a DNDarray which has a different size in the split dimension, could use cleaning.
Issue/s resolved: #825
Changes proposed:
Type of change
Due Diligence
Does this change modify the behaviour of other functions? If so, which?
no