Refactor sparse unary ops tests#49158
Conversation
…v-5/refactor-sparse-unaryops-tests
…v-5/refactor-sparse-unaryops-tests
Added operator_variant to OpInfo Updated sparse tests
…v-5/refactor-sparse-unaryops-tests
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit d352e0e (more details on the Dr. CI page): Commit d352e0e was recently pushed. Waiting for builds... 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. |
Fixed failing test_ops
…v-5/refactor-sparse-unaryops-tests
Codecov Report
@@ Coverage Diff @@
## master #49158 +/- ##
==========================================
+ Coverage 77.47% 80.75% +3.28%
==========================================
Files 1889 1975 +86
Lines 184759 216871 +32112
==========================================
+ Hits 143140 175137 +31997
- Misses 41619 41734 +115 |
…v-5/refactor-sparse-unaryops-tests
…v-5/refactor-sparse-unaryops-tests
2414b6c to
523155a
Compare
…v-5/refactor-sparse-unaryops-tests
|
Failures are all in the base, sorry about that @vfdev-5. We can ignore them and I've reverted the PR causing them. |
3f35fe0 to
4adbfca
Compare
| def _get_dense_and_sparse_samples(self, device, dtype, op): | ||
| test_backward = op.test_complex_grad or not dtype.is_complex | ||
| samples = op.sample_inputs(device, dtype, requires_grad=test_backward) | ||
| sparse_samples = op.sparse_sample_inputs(device, dtype, requires_grad=test_backward) |
There was a problem hiding this comment.
So in this model sparse_sample_inputs() are ADDITIONAL sparse-specific inputs?
| dtype = kwargs.get('dtype', torch.double) | ||
| device = kwargs.get('device', 'cpu') | ||
| singular = kwargs.get("singular", False) | ||
| values_domain = kwargs.get("domain", None) |
There was a problem hiding this comment.
In the future we should also add tests to test_testing to ensure that random_sparse_matrix() (and make_tensor()) work as expected
| dtype = kwargs.get('dtype', torch.double) | ||
| device = kwargs.get('device', 'cpu') | ||
| singular = kwargs.get("singular", False) | ||
| values_domain = kwargs.get("domain", None) |
There was a problem hiding this comment.
Why is this function **kwargs?
We don't need to fix random_sparse_matric() in this PR but that seems odd, and it makes documenting its supported kwargs tricky.
| A = torch.sparse_coo_tensor( | ||
| indices_tensor, values, (rows, columns), device=device, requires_grad=requires_grad | ||
| ) | ||
| if not uncoalesced: |
There was a problem hiding this comment.
In the future I'd like to better consider how to generate sparse tensors, especially how to handle coalescing (or not) and the density target
mruberry
left a comment
There was a problem hiding this comment.
Hey @vfdev-5!
Overall this looks pretty good. I made some inline comments/questions, mostly requesting more comments in the code to explain the choices/assumptions we're making. In the future we probably want to look more closely at generating sparse tensors, too.
facebook-github-bot
left a comment
There was a problem hiding this comment.
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Introduced operator variant to OpInfo Context: Split of pytorch#49158 cc mruberry Pull Request resolved: pytorch#50370 Reviewed By: mrshenli Differential Revision: D25897821 Pulled By: mruberry fbshipit-source-id: 4387ea10607dbd7209842b685f1794bcb31f434e
Summary: Introduced AliasInfo for OpInfo. Context: Split of pytorch#49158 cc mruberry , please let me know if you'd like to see here more code to cover > [ ] fold test_op_aliases.py into OpInfo-based testing in test_ops.py from pytorch#50006 and/or add `UnaryUfuncInfo('abs')` as discussed https://github.com/pytorch/pytorch/pull/49158/files#r548774221 Pull Request resolved: pytorch#50368 Reviewed By: ngimel Differential Revision: D26177261 Pulled By: mruberry fbshipit-source-id: 2e3884a387e8d5365fe05945375f0a9d1b5f5d82
Description:
op_dbcc @mruberry