err..what happens if you did all of this with s instead of s_values? Would it copy the indices tensor?

I think it would yeah. I think it'd also be a bit messier converting src directly since below still needs the values/indices separately, and src is a reference, so there'd be an additional temporary variable that isn't really needed.

This seems wrong because t, src could be zero-dim tensors:

i = torch.zeros([0, 1])
v = torch.tensor(3.14, dtype=torch.float32)
sparse_scalar = torch.sparse_coo_tensor(i, v, [])

Are we saying that sparse zero-dim scalars belong in a different category for type promotion?

ah I didn't realize it was possible to construct a zero-dim sparse tensor.

It is, but on the other hand, I'm not sure who does

actually, just tried this, we don't support adding dimensioned tensors to zero-dim tensors. Current implementation of sparse tensors requires t.dims == src.dims() so as long as the dims match promoteTypes will be correct anyway. We should probably expand sparse capabilities but that'll be another issue.

Cool, then there should be an assert here before the promoteTypes call so it's clear that we're assuming the inputs are in the same category

We can speed this up by allocating r_values = torch.empty_like(r_values). We don't actually care about the data that is initially in the result tensor.

Instead of doing a .to, we can save memory here by doing a copy from r_values directly into the values tensor instead of creating a new tensor here and assigning it.

I guess the values tensor of the result might not be large enough to hold all of that...

I think for this one we may still need the to, otherwise we would have to create a new empty tensor first to copy values into anyway, and I assume that's just basically the implementation of to?

r_values here is the promoted version (e.g. float += double, r_values is a double tensor, but we'd need to set r._values to a yet-uncreated float tensor containing those values.

r is the result tensor. It's possible that r already has data and r._values() and r._indices() are tensors with some non-trivial storage. If that is the case, instead of discarding values and indices in favor of the new values, we could copy r_values directly into r._values(), assuming there is enough memory in r._values().

The implementation is not the nicest; it looks like to do type promotion, the current implementation adds the logic to each possible pathway. Instead of doing that, we could imagine writing a wrapper function around the implementation that promotes the input sparse tensors to the correct dtype, calls the implementation (that operates on tensors of the same dtype), and then copy the result back to the result tensor.

yeah I'd like to clean this up eventually, but in doing so I think I'd want to extend support for more combinations of tensors too. Right now arithmetic ops on sparse tensors are pretty limited and didn't want to try to make it generic without actually making the availability of sparse ops more generic.

I don't see why "adding more support for sparse ops" and "making the implementation more generic" have to go together. Right now, the implementation for add_sparse_out_cpu is hard to review because there are two code paths that need to be considered, one for where values are contiguous, and one for when they're not.

It would make it easier to review (and also make the implementation look better) if we did something like the following (pseudocode)

add_out_sparse_cpu(result, tensor, other) {
  // do type promotion
  common_dtype = result_type(tensor, other)
  result_tmp = empty_like(result, common_dtype)
  tensor_tmp = tensor.to(common_dtype)
  other_tmp = other.to(common_dtype)

  // call the base op
  add_out_sparse_cpu_no_type_promotion(result_tmp, other_tmp, tensor_tmp)

  // shove the result back into its original type.
  result.set_inplace(result_tmp.indices(), result_tmp.values().to(result.dtype), result_tmp.sizes())
}

In general putting the type promotion code in helper functions or a wrapper function like this would help because the body of add_out_sparse_cpu as well as the other functions are really, really long

If I'm reading the code correctly, for add and mul, type promotion is implemented for the following:

• add_out_sparse (cpu + cuda)
• add_out_sparse_dense_sparse (cpu only)
• mul_out_sparse (cpu + cuda)

Why is there a discrepancy in what is supported in add_out_sparse_dense_sparse?

There are a lot of test cases here. I imagine when we add type promotion to more sparse ops, we will not want to just append LOC to this test case (test_sparse). Can we split this up into semantically meaningful tests for different ops and different overloads?