Add dparray #192

PokhodenkoSA · 2020-12-01T08:29:25Z

This code is from #184 placed in common branch.

Doc-strings for the new class and the new module should be added.

vlad-perevezentsev · 2020-12-01T11:53:55Z

Fixed comments @oleksandr-pavlyk :
#184 (review)
#184 (review)
#184 (review)

Warning and error @PokhodenkoSA (#184 (comment) ) did not find after making changes.

Commands for the conda virtual environment:
conda create -n dpctl-dev python=3.7 -c http://fxsatmac02.an.intel.com:8091
conda install cython -c http://fxsatmac02.an.intel.com:8091
conda install numpy -c http://fxsatmac02.an.intel.com:8091
conda install cmake -c http://fxsatmac02.an.intel.com:8091
export ONEAPI_ROOT=/opt/intel/oneapi
source /opt/intel/oneapi/compiler/latest/env/vars.sh
python setup.py develop

setup.py

dpctl/__init__.pxd

dpctl/tests/test_dparray.py

dpctl/dparray.py

PokhodenkoSA · 2020-12-01T16:18:10Z

Please, format code with black formatter.

dpctl/dparray.py

dpctl/tests/test_dparray.py

dpctl/__init__.pxd

dpctl/dparray.py

oleksandr-pavlyk · 2020-12-02T23:11:22Z

dpctl/dparray.py

+        return self
+
+    def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
+        if method == "__call__":


Need to also handle method="accumulate" , "reduce", "reduceat", "outer"

Can we add a TODO and open a ticket to track this future work.

dpctl/dparray.py

… MemInfo. We'll have to add __sycl_usm_array_interface__ property to Numba objects to get this equivalent behavior without this dependency in the wrong direction.

diptorupd · 2020-12-03T04:18:56Z

one general comment; I want to add the new dparray class under dpctl.dptensor.dparray sub module. @oleksandr-pavlyk suggested that we can have other types of tensors defined in dpctl later and it makes sense to keep all of them in one place under a dptensor sub module.

setup.py

PokhodenkoSA · 2020-12-03T07:31:52Z

dpctl/tests/test_dparray.py

-    def test_dparray_through_python_func(self):
-        def func_operation_with_const(dpctl_array):
-            return dpctl_array * 2.0 + 13
-
-        C = self.X * 5
-        dp_func = func_operation_with_const(C)
-        self.assertIsInstance(dp_func, dparray.ndarray)
-


Why this test is deleted?
@vlad-perevezentsev

PokhodenkoSA · 2020-12-03T07:36:47Z

add the new dparray class under dpctl.dptensor.dparray sub module

@vlad-perevezentsev Please, make it in separate PR.

PokhodenkoSA

LGTM.

PokhodenkoSA · 2020-12-04T15:31:15Z

add the new dparray class under dpctl.dptensor.dparray sub module

@vlad-perevezentsev Please, make it in separate PR.

#200

MANIFEST.in

dpctl/dparray.py

diptorupd · 2020-12-04T18:35:47Z

dpctl/dparray.py

+        return self
+
+    def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
+        if method == "__call__":


Can we add a TODO and open a ticket to track this future work.

oleksandr-pavlyk · 2020-12-04T23:09:13Z

We should try to merge PRs Add dparray #192 and Add the new dparray class under dpctl.dptensor.dparray #200 into a single PR before moving on, these PRs are interdependent.
In dpctl/dptensor/__init__.py we should avoid doing from .dparray import * because it pulls the entire namespace of NumPy. Instead we should do from .dparray import ndarray as ndarray_USMShared. It would best reflect implementation if dparray.py is renamed to _ndarray_subclass.py.
Tests need to be adjusted accordingly.
Doc-strings for the new class and the new module should be added.
I will work on __array_ufunc__ in a separate PR.

DrTodd13 · 2020-12-04T23:20:32Z

We should try to merge PRs #192 and #200 into a single PR before moving one, these PRs are interdependent.

In dpctl/dptensor/__init__.py we should avoid dong from .dparray import * because it pull the entire namespace of NumPy.
Instead we should do from .dparray import ndarray as ndarray_USMShared.

It would best reflect implementation is dparray.py is renamed to _ndarray_subclass.py.

I would like a name that reflects that it can be imported as a full numpy replacement. "dparray" doesn't quite capture that but "_ndarray_subclass" I think could even more so give the wrong impression. At some point we want to call dpnp from there so you're going to get much more than just an ndarray replacement.

diptorupd · 2020-12-04T23:25:45Z

dpctl/dparray.py

+
+    # Tell Numba to not treat this type just like a NumPy ndarray but to propagate its type.
+    # This way it will use the custom dparray allocator.
+    __numba_no_subtype_ndarray__ = True


@DrTodd13 Do we really need this extra attribute? Can we not use the facts that the array is a subclass of NumPy.ndarray and it defines the __sycl_usm_array_interface__ to recognize this class inside Numba?

Numba is currently made to treat Numpy.ndarray subclasses as if they were Numpy.ndarrays. Therefore, we can't just check for whether it is a ndarray subclass to keep those types separate without breaking backwards compatibility.

Also, you might want to keep types separate even if you don't need a special allocator. So, for generality, we shouldn't specialize on something allocator or USM-specific inside Numba.

diptorupd · 2020-12-05T00:43:31Z

We should try to merge PRs #192 and #200 into a single PR before moving one, these PRs are interdependent.
In dpctl/dptensor/__init__.py we should avoid dong from .dparray import * because it pull the entire namespace of NumPy.
Instead we should do from .dparray import ndarray as ndarray_USMShared.
It would best reflect implementation is dparray.py is renamed to _ndarray_subclass.py.

I would like a name that reflects that it can be imported as a full numpy replacement. "dparray" doesn't quite capture that but "_ndarray_subclass" I think could even more so give the wrong impression. At some point we want to call dpnp from there so you're going to get much more than just an ndarray replacement.

PokhodenkoSA · 2020-12-05T13:34:06Z

@diptorupd Why did you close this PR? What is the plan for it now?
@vlad-perevezentsev spend significant part of the week to prepare this PR to merge.
#200 was targeted to merge to #192.

diptorupd · 2020-12-07T14:28:17Z

@PokhodenkoSA not sure when/what I did. Must have accidentally clicked wrong button.

MANIFEST.in

Remove `include dparray.py`

PokhodenkoSA · 2020-12-07T14:54:15Z

We should try to merge PRs Add dparray #192 and Add the new dparray class under dpctl.dptensor.dparray #200 into a single PR before moving on, these PRs are interdependent.

Not necessary. #200 could be merged later. #200 depends on #192. #200 is independent.
Now numba and numba-dppy has PR which expects dpctl.dparray exists. It should be updated too. I would like to move step by step and do not add dptensor module until we have nothing to add also.
In any case we can do in dpctl/__init__.py replase from . import dparray with from .dptensor import dparray and propose clinents to use from dpctl import dparray.

@DrTodd13

This PR contains all the changes in #192, #200. I will close those two PRs after merging this one. Pending issues to improve the dparray implementation are to be addressed as follow up PRs. Thanks @DrTodd13 @oleksandr-pavlyk @vlad-perevezentsev for working on this PR.

DrTodd13 and others added 2 commits November 25, 2020 09:08

Initial separation of dparray into 2 parts.

d2458eb

Merge branch 'master' into spokhode/dparray

6b892ea

PokhodenkoSA assigned vlad-perevezentsev Dec 1, 2020

fix comments

7311b94

PokhodenkoSA commented Dec 1, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

add tests for dparray

fb163ec

PokhodenkoSA commented Dec 1, 2020

View reviewed changes

dpctl/__init__.pxd Outdated Show resolved Hide resolved

PokhodenkoSA commented Dec 1, 2020

View reviewed changes

dpctl/tests/test_dparray.py Outdated Show resolved Hide resolved

PokhodenkoSA commented Dec 1, 2020

View reviewed changes

dpctl/dparray.py Outdated Show resolved Hide resolved

PokhodenkoSA commented Dec 2, 2020

View reviewed changes

dpctl/dparray.py Outdated Show resolved Hide resolved

PokhodenkoSA commented Dec 2, 2020

View reviewed changes

dpctl/tests/test_dparray.py Outdated Show resolved Hide resolved

PokhodenkoSA commented Dec 2, 2020

View reviewed changes

dpctl/tests/test_dparray.py Outdated Show resolved Hide resolved

vlad-perevezentsev added 3 commits December 2, 2020 08:50

fix comments

f0c3c2b

use black

935a2a1

fix the error from black and remove one test

efcfcd9

diptorupd reviewed Dec 2, 2020

View reviewed changes

dpctl/__init__.pxd Outdated Show resolved Hide resolved

oleksandr-pavlyk reviewed Dec 2, 2020

View reviewed changes

dpctl/dparray.py Outdated Show resolved Hide resolved

oleksandr-pavlyk reviewed Dec 2, 2020

View reviewed changes

dpctl/dparray.py Outdated Show resolved Hide resolved

oleksandr-pavlyk reviewed Dec 2, 2020

View reviewed changes

dpctl/dparray.py Outdated Show resolved Hide resolved

Resolve some of the code review issues. Not explicitly checking Numba…

7ebd06c

… MemInfo. We'll have to add __sycl_usm_array_interface__ property to Numba objects to get this equivalent behavior without this dependency in the wrong direction.

PokhodenkoSA commented Dec 3, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

PokhodenkoSA commented Dec 3, 2020

View reviewed changes

PokhodenkoSA added 3 commits December 4, 2020 08:16

Add test_dparray.py into test system

65925b6

Rename test class for dparray

ef2902f

Merge branch 'master' into spokhode/dparray

a4907dd

PokhodenkoSA added 3 commits December 4, 2020 08:20

Remove formatting of setup.py

3c69ed7

Add license in new files.

ea5e319

Remove import dparray from __inti__.pxd

5fbef1c

PokhodenkoSA commented Dec 4, 2020

View reviewed changes

diptorupd reviewed Dec 4, 2020

View reviewed changes

diptorupd closed this Dec 5, 2020

PokhodenkoSA mentioned this pull request Dec 7, 2020

Add the new dparray class under dpctl.dptensor.dparray #200

Closed

diptorupd reopened this Dec 7, 2020

PokhodenkoSA commented Dec 7, 2020

View reviewed changes

MANIFEST.in Outdated Show resolved Hide resolved

Update MANIFEST.in

897afc9

Remove `include dparray.py`

oleksandr-pavlyk mentioned this pull request Dec 7, 2020

Merged 192 200 #204

Merged

diptorupd merged commit 44bbe03 into master Dec 7, 2020

PokhodenkoSA mentioned this pull request Dec 9, 2020

Initial support for Numpy subclasses (for dparray) IntelPython/numba#116

Closed

2 tasks

Add dparray #192

Add dparray #192

Uh oh!

Conversation

PokhodenkoSA commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlad-perevezentsev commented Dec 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PokhodenkoSA commented Dec 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleksandr-pavlyk Dec 2, 2020

Choose a reason for hiding this comment

Uh oh!

diptorupd Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

diptorupd commented Dec 3, 2020

Uh oh!

Uh oh!

PokhodenkoSA Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

PokhodenkoSA commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PokhodenkoSA left a comment

Choose a reason for hiding this comment

Uh oh!

PokhodenkoSA commented Dec 4, 2020

Uh oh!

Uh oh!

Uh oh!

diptorupd Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

oleksandr-pavlyk commented Dec 4, 2020 • edited by diptorupd Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrTodd13 commented Dec 4, 2020

Uh oh!

diptorupd Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

DrTodd13 Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

diptorupd commented Dec 5, 2020

Uh oh!

PokhodenkoSA commented Dec 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diptorupd commented Dec 7, 2020

Uh oh!

Uh oh!

PokhodenkoSA commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

PokhodenkoSA commented Dec 1, 2020 •

edited

Loading

PokhodenkoSA commented Dec 3, 2020 •

edited

Loading

oleksandr-pavlyk commented Dec 4, 2020 •

edited by diptorupd

Loading

PokhodenkoSA commented Dec 5, 2020 •

edited

Loading

PokhodenkoSA commented Dec 7, 2020 •

edited

Loading