Skip to content
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

Implement support of tuple key in __getitem__ and __setitem__ #1362

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

antonwolfy
Copy link
Contributor

@antonwolfy antonwolfy commented Apr 3, 2023

The PR is about to close #1361.

It introduced support of key as a tuple in dpnp.__getitem__() and dpnp.__setitem__() functions.

Previously dpnp was falling back on dpctl call in case of tuple as a key, but it is not always correct.
It requires to unwrap dpnp array into USM ndarray for each dpnp array in tuple key before invoking dpctl functions.

  • Have you provided a meaningful PR description?
  • Have you added a test, reproducer or referred to issue with a reproducer?
  • Have you tested your changes locally for CPU and GPU devices?
  • Have you made sure that new changes do not introduce compiler warnings?
  • If this PR is a work in progress, are you filing the PR as a draft?

@oleksandr-pavlyk
Copy link
Contributor

By the way, using test_equal may not be the most efficient way of testing array equality:

In [13]: import dpnp, numpy as np

In [14]: x = dpnp.arange(6)

In [15]: np.asarray(x)
Out[15]:
array([array(0), array(1), array(2), array(3), array(4), array(5)],
      dtype=object)

In [16]: x = dpnp.arange(6).reshape((2,3))

In [17]: np.asarray(x)
Out[17]:
array([[array(0), array(1), array(2)],
       [array(3), array(4), array(5)]], dtype=object)

I can not quite put my finger on what property of the container makes NumPy behave like this.

I have tried to create a mock indexable array, but it behaves different:


In [24]: class Indexable:
    ...:     def __init__(self, n):
    ...:         self.data_ = list(range(n))
    ...:     def __getitem__(self, key):
    ...:         return self.data[key]
    ...:     def __setitem__(self, key, val):
    ...:         self.data_[key] = val
    ...:     @property
    ...:     def shape(self):
    ...:         return (len(self.data_),)
    ...:     @property
    ...:     def dtype(self):
    ...:         return np.int_
    ...:

In [25]: np.asarray(Indexable(6))
Out[25]: array(<__main__.Indexable object at 0x7f5d742c7790>, dtype=object)

@antonwolfy
Copy link
Contributor Author

By the way, using test_equal may not be the most efficient way of testing array equality:

In [13]: import dpnp, numpy as np

In [14]: x = dpnp.arange(6)

In [15]: np.asarray(x)
Out[15]:
array([array(0), array(1), array(2), array(3), array(4), array(5)],
      dtype=object)

In [16]: x = dpnp.arange(6).reshape((2,3))

In [17]: np.asarray(x)
Out[17]:
array([[array(0), array(1), array(2)],
       [array(3), array(4), array(5)]], dtype=object)

I can not quite put my finger on what property of the container makes NumPy behave like this.

I have tried to create a mock indexable array, but it behaves different:


In [24]: class Indexable:
    ...:     def __init__(self, n):
    ...:         self.data_ = list(range(n))
    ...:     def __getitem__(self, key):
    ...:         return self.data[key]
    ...:     def __setitem__(self, key, val):
    ...:         self.data_[key] = val
    ...:     @property
    ...:     def shape(self):
    ...:         return (len(self.data_),)
    ...:     @property
    ...:     def dtype(self):
    ...:         return np.int_
    ...:

In [25]: np.asarray(Indexable(6))
Out[25]: array(<__main__.Indexable object at 0x7f5d742c7790>, dtype=object)

There will be dpnp.dpnp_utils.convert_item function called to convert to numpy array, before invoking numpy.testing.assert_equal. Thus dtype will be correct one.

@antonwolfy antonwolfy merged commit 2168093 into IntelPython:master Apr 4, 2023
@antonwolfy antonwolfy deleted the tuple_as_index_key branch April 4, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improper handling of key as a tuple in dpnp_array.__getitem__()
2 participants