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

In-place operations #80

Closed
hameerabbasi opened this Issue Jan 16, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@hameerabbasi
Collaborator

hameerabbasi commented Jan 16, 2018

I was wondering about operators such as operator.iadd, etc. There are a few ways we can go about this:

  1. Support them by mutating the object in-place and invalidating the cache, i.e. performing the operation and then making self a copy of the returned object.
  2. Support them only when the sparsity structure is the same, and modify data in-place.
  3. Don't support them at all.

If we want to maintain compatibility with Numpy code (I hope to make COO a mostly drop-in replacement for ndarray with a few exceptions at some point), I would go with 1, with a warning in the docs that in-place isn't really "in-place".

If we want to do our own thing... Then we have options 2 and 3.

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented Jan 29, 2018

cc @mrocklin @nils-werner Your thoughts would be welcome here.

@nils-werner

This comment has been minimized.

Contributor

nils-werner commented Jan 29, 2018

The docs say about inplace modification of immutable vs mutable types:

x += y is equivalent to x = operator.iadd(x, y)

and

For immutable targets such as strings, numbers, and tuples, the updated value is computed, but not assigned back to the input variable:

from operator import iadd

a = 'hello'                 # hello
iadd(a, ' world')           # hello world
a                           # hello

For mutable targets such as lists and dictionaries, the inplace method will perform the update, so no subsequent assignment is necessary:

s = list('hello')           # hello
iadd(s, list(' world'))     # hello world
s                           # hello world

To me, that sounds like solution 1. would be acceptable behaviour.

Another solution that comes to mind would be to keep self as it is and only replace shape, coords and data.

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented Jan 29, 2018

Another solution that comes to mind would be to keep self as it is and only replace shape, coords and data.

coords and data I agree on. shape, I'm not so sure about. Take the following simple example using Numpy (they don't support assigning when the shape isn't broadcastable to the output shape):

>>> import numpy as np
>>> x = np.zeros((1, 5))
>>> y = np.ones((5, 5))
>>> x += y
Traceback (most recent call last):
  File "<input>", line 1, in <module>
ValueError: non-broadcastable output operand with shape (1,5) doesn't match the broadcast shape (5,5)
@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented Jan 29, 2018

Option 1 seems fine with me with a couple of small comments:

  1. We should continue to fail when we would have failed before, like in x += 1 (because this would densify)
  2. We might consider operating genuinely in place when it's convenient. This is often the case when operating with scalars like x *= 2 or x /= 2

@hameerabbasi hameerabbasi modified the milestones: 0.3, 0.4 Feb 22, 2018

@hameerabbasi hameerabbasi referenced this issue May 3, 2018

Merged

Inplace ops #146

@mrocklin mrocklin closed this in #146 May 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment