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

Add cupy.piecewise #3329

Merged
merged 30 commits into from Jun 11, 2020
Merged

Add cupy.piecewise #3329

merged 30 commits into from Jun 11, 2020

Conversation

Dahlia-Chehata
Copy link
Contributor

No description provided.

@Dahlia-Chehata Dahlia-Chehata changed the title initial commit Piecewise May 11, 2020
@Dahlia-Chehata
Copy link
Contributor Author

n numpy cupy
100 195.004 us 413.825 us
1000 1924.999 us 1338.802 us
10000 18665.776 us 10674.284 us
100000 186747.146 us 103642.666 us
1000000 1930592.689 us 1049231.745 us

@Dahlia-Chehata
Copy link
Contributor Author

Is there a way to support callable functions with kernels ?
to parallelize this loop :
if any(isinstance(item, Callable) for item in funclist):
raise ValueError('Callable functions are not supported')

@Dahlia-Chehata
Copy link
Contributor Author

n numpy cupy
100 207.422 385.469
1000 2002.841 1064.974
10000 19664.703 7700.720
100000 195738.325 73730.930
1000000 1968100.123 732554.892

Copy link
Member

@asi1024 asi1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update a document in docs/source/reference.

cupy/__init__.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
tests/cupy_tests/functional_tests/test_piecewise.py Outdated Show resolved Hide resolved
tests/cupy_tests/functional_tests/test_piecewise.py Outdated Show resolved Hide resolved
@asi1024
Copy link
Member

asi1024 commented May 12, 2020

@Dahlia-Chehata Could you show us the benchmark script?

@asi1024 asi1024 changed the title Piecewise Add cupy.piecewise May 12, 2020
@asi1024 asi1024 added the cat:feature New features/APIs label May 12, 2020
@asi1024 asi1024 added this to the v8.0.0b3 milestone May 12, 2020
@Dahlia-Chehata
Copy link
Contributor Author

@Dahlia-Chehata Could you show us the benchmark script?

def compare(n):
    def generator(n, p=50):
        x = np.linspace(-50, 50, p)
        funclist = [random.randint(1000) for x in range(1, n)]
        condlist=[]
        for i in range(len(funclist)):
            condlist.append([bool(getrandbits(1))for j in range (len(x))])
        return x, funclist, condlist
    x1, f1, c1 = generator(n,np)
    x2, f2, c2 = generator(n,cp)
    np_ret = repeat(np.piecewise, (x1, c1, f1), n=10, n_warmup=10)
    cp_ret = repeat(cp.piecewise, (x2, c2, f2), n=10, n_warmup=10)
    print("NumPy: ", np_ret.to_str(show_gpu=True))
    print("CuPy:", cp_ret.to_str(show_gpu=True)) 

cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
tests/cupy_tests/functional_tests/test_piecewise.py Outdated Show resolved Hide resolved
tests/cupy_tests/functional_tests/test_piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
@Dahlia-Chehata
Copy link
Contributor Author

@asi1024 Can you please review ?

cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
condlist = cupy.asarray(condlist)
condlen = len(condlist)
if diffshape:
if x.ndim == condlist.ndim:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for x.ndim != condlist.ndim case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it already checked in this else ?

else:
    raise IndexError('boolean index did not match indexed array along '
    'dimension 0; dimension is {} but corresponding '
    'boolean dimension is {}'
    .format(x.shape[0], condlen))

I mean for example the first case will pass and the second will raise the error

x = testing.shaped_arange((2, 3), xp, dtype)
condlist = [False, True]
funclist = [1, 2]
xp.piecewise(x, condlist, funclist)
x = testing.shaped_arange((2, 3), xp, dtype)
condlist = [False, True, True]
funclist = [1, 2]
xp.piecewise(x, condlist, funclist)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why should we check the shape equality only when x.ndim == condlist.ndim?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question. Why you don't check the shape when diffshape == 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why should we check the shape equality only when x.ndim == condlist.ndim?

I thought I explained it by the above examples. When x.ndim == condlist.ndim, we should check for the shapes. By not satisfying (x.dim == conlist.ndim) condition, only 2 options are possible now, either condlen == x.shape[0] (and in this case condlen should be adjusted to 1 to skip the error in L74 or simply to mean that a special broadcasting will be used for this case) or raise the error because of index mismatch along dimension 0 as it can be seen from the examples I explained here
Does this answer the question ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you check the shape equality only when x.ndim == condlist.ndim, and don't check when x.ndim != condlist.ndim?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question. Why you don't check the shape when diffshape == 0?

diffshape flag (as by its name) already targets special cases that need a different broadcasting ( list of scalars with multidimensional input or list of tuples/ nested tuples with multidimensional input). Handling these special cases require a pre-step before resuming the normal behavior for all (diffshape == 1, 2 or 0). This extra step can be seen by entering if diffshape: condition. After that, the default behaviour is applied for different inputs regardless of diffshape value ( that can be seen from line 61, and of course with the exception of transposing condlist in case of diffshape == 1 later on...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you check the shape equality only when x.ndim == condlist.ndim, and don't check when x.ndim != condlist.ndim?

because I don't need the shapes information when x.ndim != condlist.ndim
Either condlen == x.shape[0] and here special broadcasting is used

x = testing.shaped_arange((2, 3), xp, dtype)
condlist = [False, True]
funclist = [1, 2]
xp.piecewise(x, condlist, funclist) 

or raise error because of mismatch along dimension 0

condlist = [False, True, True]
funclist = [1, 2]
xp.piecewise(x, condlist, funclist)

I only care about the shape along dimension 0 when x.ndim != condlist.ndim

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some different behaviors between this implementation and NumPy's one.

  • Only cupy raises ValueError.
    @testing.for_all_dtypes()
    @testing.numpy_cupy_array_equal()
    def test_broadcast(self, xp, dtype):
        x = testing.shaped_random(shape=(2, 3, 5), xp=xp, dtype=dtype)
        condlist = [[[True, False, False], [True, True, True]]]
        funclist = [-1, 2]
        return xp.piecewise(x, condlist, funclist)
  • Only numpy raises error.
    @testing.for_all_dtypes()
    @testing.numpy_cupy_array_equal()
    def test_broadcast(self, xp, dtype):
        x = testing.shaped_random(shape=(2, 3, 5), xp=xp, dtype=dtype)
        condlist = [[[True, False, False, False, False]], [[True, True, True, True, True]]]
        funclist = [-1, 2]
        return xp.piecewise(x, condlist, funclist)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into it

cupy/functional/piecewise.py Outdated Show resolved Hide resolved
@Dahlia-Chehata
Copy link
Contributor Author

@asi1024 Could you help with travis ? It seems like a common issue

Comment on lines 37 to 42
if cupy.isscalar(condlist):
condlist = [condlist]
elif isinstance(condlist[0], tuple) and x.ndim != 0:
diffshape = 2
elif cupy.isscalar(condlist[0]) and x.ndim != 0:
diffshape = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please enhance the coverage of all types of given condlist. For example, IndexError will be raised when condlist is 0-dim ndarray.

cupy/functional/piecewise.py Outdated Show resolved Hide resolved
cupy/functional/piecewise.py Outdated Show resolved Hide resolved
condlist = cupy.asarray(condlist)
condlen = len(condlist)
if diffshape:
if x.ndim == condlist.ndim:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question. Why you don't check the shape when diffshape == 0?

@asi1024
Copy link
Member

asi1024 commented Jun 10, 2020

Jenkins, test this please.

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 72035ef:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 72035ef, target branch master) failed with status FAILURE.

@asi1024
Copy link
Member

asi1024 commented Jun 11, 2020

Jenkins, test this please.

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 38cd394:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 38cd394, target branch master) succeeded!

@asi1024
Copy link
Member

asi1024 commented Jun 11, 2020

LGTM.

@asi1024 asi1024 merged commit 9ec3bd3 into cupy:master Jun 11, 2020
@Dahlia-Chehata Dahlia-Chehata deleted the piecewise branch July 6, 2020 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:feature New features/APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants