nonzero, where fixes from #938#2332
Conversation
| ) | ||
| # vectorized sorting of nz indices along axis 0 | ||
| global_nonzero.balance_() | ||
| global_nonzero = manipulations.unique(global_nonzero, axis=0) |
There was a problem hiding this comment.
Why is this needed? Seems like duplicate entries would be a bug at this point. Or are there some side effects of unique here?
| self.assertEqual(len(wh), 2) | ||
| self.assertEqual(wh[0].gshape[0], 6) | ||
| self.assertEqual(wh[0].dtype, ht.int64) | ||
| self.assertEqual(wh[0].split, None) |
There was a problem hiding this comment.
Maybe this could be compared to numpy and torch too?
|
The issues arise because the API of nonzero is totally changed in this PR. See the following example: import heat as ht
import torch
import numpy as np
a = ht.arange(7)
print(torch.nonzero(a.larray > 3))
# tensor([[4],
# [5],
# [6]])
print(np.nonzero(a.numpy() > 3))
# (array([4, 5, 6]),)
print(ht.nonzero(a > 3))
# on main: DNDarray([4, 5, 6], dtype=ht.int64, device=cpu:0, split=None)
# on features/nonzero-updates: (DNDarray(MPI-rank: 0, Shape: (3,), Split: None, Local Shape: (3,), Device: cpu:0, Dtype: int64, Data:
# [4, 5, 6]),)
# new on features/nonzero-updates:
print(ht.nonzero(a > 3, as_tuple=False))
# DNDarray([[4],
# [5],
# [6]], dtype=ht.int64, device=cpu:0, split=None)That is to say the previous API was neither numpy, nor torch and this PR changes it to default to numpy but with the option to get the torch behavior. This is a good thing in principle, but I don't like to silently change the API. So, yes it's annoying that the current main relies on the previous API in a bunch of places and doesn't work with only these changes, but also we should clearly indicate this as a breaking, API-changing change. We have two options:
Either way, we need to mention the API changes in the release notes. What do you think, @JuanPedroGHM, @mtar, @ClaudiaComito? |
2 participants
Due Diligence
Description
Issue/s resolved: #
Changes proposed:
Type of change
Memory requirements
Performance
Does this change modify the behaviour of other functions? If so, which?
yes / no