Add option to avoid deep copy in tensor.double

[tgkolda]

The tensor.double function returns a deep copy of the tensor. I'm not sure that's always necessary. I suggest that we extend the function to specify whether or not to create a deep copy. It can default to true, but I think there will be instances where that behavior is not desired.

[ntjohnson1]

I agree that someone might want access to the underlying numpy data without making a deep copy. My initial intuition is not to overload double for this. Since our other tensors also have double methods this seems to confuse the interface (since ktensor and sptensor need to allocate new memory to yield a dense representation). Right now the underlying data buffer is publicly available, so users can access that (but not the most ergonomic). Otherwise I think double_view or some other more explicit name that only tensor has might be nicer.

Then we could basically have tensor double call double_view and make a copy if necessary. This would also save us a copy on ktensor double where we call full then double.

Open to additional perspective if there's a specific use case in mind or confusion you ran into.

[tgkolda]

Double serves a variety of purposes. At its heart, it extracts the underlying ndarray. There is no reason to do anything but a view.

However, if that underlying array is not already a double or doesn't exist, then it does the conversion.

We really should avoid memory copies as a general principle and force he user to make explicit copies if needed. 

[ntjohnson1]

If tensor provides a view to the underlying data then double functions differently under tensor vs sptensor/ktensor.

X = dense_tensor.double()
Y = a_ktensor.double()

Mutating X changes the underlying dense_tensor where mutating Y doesn't change the underlying ktensor. If the user always just made a copy of the value returned by double then the non-tensor versions would have an unnecessary copy.

IIUC MATLAB does copy on write, so if not mutating things acts as a view and makes the copy on mutation. However numpy can either provide a view or do the copy at creation. Agreed we should avoid unnecessary copies, but we also want to be consistent/clear. Definitely more work to do around that.

[tgkolda]

I understand what you are saying, but I recommend that the python implementation adopt the default of avoiding copies whenever possible. I'm arguing for efficiency to trump consistency. Essentially, if the data is already a double ndarray, it returns a view. In any other situation, it creates a new array and returns that. I'm okay with splitting double into double_view and double_copy, but then we need to have these functions for every class, even though most classes won't have a view. This is why I would suggest just have a flag to double. This would be easy to implement in all classes for consistency, and then it would enable the user to only create a view when that's all that's needed.

[ntjohnson1]

I think if we could avoid copies for all implementations of double then that would make sense. But since we can't it increases complexity to treat them differently across the classes. if the data is already a double ndarray has an assumption about the implementation that the interface alone doesn't enforce.

I think the whole idea of adding double_view is to NOT add it to the other classes. Each tensor type could have their own specialized methods for optimizing things like this, but methods that are shared across all of them behavior in a consistent manner. If a developer wants this optimization then it's a very clear opt in that only works for tensor where it is viable. I guess this is functionally quite similar to only adding the flag to double for the tensor class, but is clearer IMO.

[tgkolda]

The reason that the interface needs to be consistent across all the classes is so that we don't have to specialize the code by the type of tensor. If I know I need a double array version of the tensor, it shouldn't matter what the class is. If I know I'm not going to modify that array, I should be able to further request a view (if possible) in a way that works with all the classes so no specialization of the code is necessary for the specific tensor types.

[ntjohnson1]

Ok, if we make the flag named immutable then we can set the writeable flag on the returned array to false so there's a runtime error if someone tries to write to it. That would function the same across all classes and specifically for tensors if we know they can't mutate the return then a view is safe. I think that strikes the consistency/efficiency tradeoff and doesn't introduce a big foot gun.

X = any_tensor.double()
X[0] = 1 # fine
Y = any_tensor.double(immutable=True)
Y[0] = 1 # runtime error about read only
# technically still possible but not likely/ feels very deliberate
Y.writeable = True
Y[0] = 1 # could mutate any_tensor

[tgkolda]

Does setting immutable affect any_tensor itself?

[ntjohnson1]

No. It would only impact the ability to modify the value returned.

In case concreteness helps for non-dense tensors the implementation is directionally

def double(self, immutable=False):
    return_array = <current_code>
    if immutable:
       return_array.writable=False
    return return_array

For dense tensors

def double(self, immutable=False):
   # if the data type and order are already correct then when set to immutable this should just be a view
   # will need to add a test to confirm this and potentially small refactor
   return_array = self.data.astype(np.float64, order=self.order, copy=not immutable)
   if immutable:
      # This is a flag just on this particular view
      return_array.writable = False
   return return_array

[tgkolda]

That's super!

[dmdunla]

Closed by d5dc655