[wasm] Add sparseToDense kernel #6528
Conversation
| if (!update_as_scalar) { | ||
| updates_ptr++; | ||
| } |
There was a problem hiding this comment.
I don't fully understand the purpose of this. It looks like it's only used if sparse_values_rank == 0 (SparseToDense.cc:35), in which case it assigns all the values of the output tensor to *updates_ptr (without incrementing).
Since this only happens when the rank of the sparse tensor is zero, wouldn't this only ever assign one value? In that case, I don't think it's necessary, since the loop will only run once.
On the other hand, it's implemented the same way in the CPU backend, so I think this is fine.
There was a problem hiding this comment.
TensorFlow doc states that if sparse_values is scalar, the value is assigned to all sparce indices.
So as far as I understand, sparse_indices could take more than one index while sparse_values is a scalar, and the loop will in turn run more than once.
| } | ||
| } | ||
|
|
||
| out_buf_ptr -= (flattened_index * slice_size + slice_size); |
There was a problem hiding this comment.
This is very confusing since you are technically resetting the pointer but using decrement. Can you leave out_buf_ptr unchanged and create a new variable that stores the start of the current iteration output (i.e. out_buf_ptr + flattened_index * slice_size) and only increment the temporary so this decrement reset is no longer necessary?
|
Would love to get this merged as it's also a pain point for my work. Given the lack of activity, is it possible to merge despite @ahmedsabie nit? If not, I'm happy to try to resolve in a separate PR, although I've never worked with the C++ implementation and Bazel. Are there up to date instructions on building from source anywhere? My (albeit brief) search came up empty. |
|
Sorry I didn't follow up on this earlier. Ahmed has left the TFJS team, so I'll assign this to someone else. |
mattsoulanille
requested review from
chunnienc and
ahmedsabie
and removed request for
ahmedsabie
| auto& sparse_values_info = backend::get_tensor_info(sparse_values_id); | ||
| auto& default_value_info = backend::get_tensor_info(default_value_id); | ||
| const std::vector<size_t>& strides = | ||
| std::vector<size_t>(strides_ptr, strides_ptr + slice_rank); |
There was a problem hiding this comment.
const std::vector<size_t> strides(strides_ptr, strides_ptr + slice_rank);
It's bad to keep the lvalue ref of a temporary value.
There was a problem hiding this comment.
I think it's valid because the lifetime of the temporary object will be extended by binding to const ref.
Anyway, the reference here was completely redundant and your suggested change is much clearer. Thank you!
|
Thanks for the quick follow up @mattsoulanille and review @chunnienc. @kon72 do you still have interest / availability to address the review? |
|
Alright @mattsoulanille, I've opened a PR on @kon72's fork that addresses @chunnienc's change request. Are you able to merge that PR to the fork as you have maintainer rights? Otherwise I'm happy to open a clean PR with my fork. |
|
@sklum Thanks for the fix. I can't merge the PR on the fork, but I cherry-picked the commit and pushed it here for @chunnienc to take a look at. |
|
Hello all, Sorry for the long delay and thank you for reviewing and following up on this PR. |
Closes #4824
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is