Added sparse-tensor copy logic to dispatcher #65304
Conversation
- Only ported copy for sparse tensor to dispatcher. Everything else is the same - Duplicated code for named tensor handling in sparse tensor copy - Might change it later to handle named tensors using dispatcher
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit fc729c6 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Codecov Report
@@ Coverage Diff @@
## master #65304 +/- ##
==========================================
- Coverage 66.45% 66.45% -0.01%
==========================================
Files 735 735
Lines 93906 93906
==========================================
- Hits 62402 62401 -1
- Misses 31504 31505 +1 |
| bool non_blocking) { | ||
| // TODO: Once copy_ is fully migrated to use dispatcher, handle named | ||
| // inference using dispatcher instead of doing it everywhere | ||
| auto maybe_outnames = namedinference::compute_broadcast_outnames(self, src); |
There was a problem hiding this comment.
Is this making copy sparse more correct than it was before? Because in the old code it seems like names were just not propagated for sparse at all. I think I'd probably prefer that, mostly because it's not worth gooping up the code here.
There was a problem hiding this comment.
And then maybe you can use copy_sparse_to_sparse_; so instead of double dispatch (go to wrapper, than go to copy sparse to sparse), just inline the dispatch table of copy_sparse_to_sparse into copy.
There was a problem hiding this comment.
Uh....I thought the sparse flow was going through name propagation. It went through copy_ which did name prop and called copy_impl containing all implementations.
Please correct me if I'm wrong
ezyang
changed the title
|
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
4 participants
Issue #61122