Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bsr_dense_mm(): better test coverage #100543

Closed
wants to merge 19 commits into from

Conversation

nikitaved
Copy link
Collaborator

@nikitaved nikitaved commented May 3, 2023

This PR improves test coverage for bsr_dense_mm by:

  • enabling correctness tests for float32.
  • extending and testing input correctness checks.

Stack from ghstack (oldest at bottom):

cc @alexsamardzic @pearu @cpuhrsch @amjames @bhosmer

@pytorch-bot
Copy link

pytorch-bot bot commented May 3, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/100543

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit ddb3f0a:
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the release notes: sparse release notes category label May 3, 2023
nikitaved added a commit that referenced this pull request May 3, 2023
ghstack-source-id: e31a77f85f1baaf0fdcd29d8fb71008d98113549
Pull Request resolved: #100543
@nikitaved nikitaved added the module: sparse Related to torch.sparse label May 3, 2023
@nikitaved nikitaved requested a review from cpuhrsch May 3, 2023 14:38
@nikitaved nikitaved added the ciflow/trunk Trigger trunk jobs on your pull request label May 3, 2023
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 4, 2023
ghstack-source-id: 71b3ae9c26eb9e974453458af2ac0489cda374c6
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 4, 2023
ghstack-source-id: 4e95280522b9e60764e0b4b027cb89b5a1f026ca
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 4, 2023
ghstack-source-id: a804f5d01e00004abb496994875b8e53a7488c77
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 4, 2023
ghstack-source-id: 54817c0eea5ff707ba16c815f3e5485dc3101fbc
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 4, 2023
ghstack-source-id: f97f73bb21400ecbc695ffe5782e2b28f5d20c5b
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 5, 2023
ghstack-source-id: 0d110e6cb320aec10dc7a5b4d73a791ab974bbc3
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 5, 2023
ghstack-source-id: 8abe3a852d82a521fdb1c7d4fe8e3d3faca03689
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 8, 2023
ghstack-source-id: 0debfbc26b7f1f01ff936b0eacdaf5cb13e81b43
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 8, 2023
ghstack-source-id: 2bcd8a6281a1ace6da686838d081e5dea6e1a980
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
nikitaved added a commit that referenced this pull request May 8, 2023
ghstack-source-id: 2294634372f8b2c8b453bbfbffbbed8f7a483b2f
Pull Request resolved: #100543
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
This PR improves test coverage for `bsr_dense_mm` by:
- enabling correctness tests for `float32`
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
This PR improves test coverage for `bsr_dense_mm` by:
- ~~enabling correctness tests for `float32`~~.
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
test/test_sparse_csr.py Outdated Show resolved Hide resolved
Comment on lines +3415 to +3417
if torch.cuda.device_count() > 1:
with self.assertRaisesRegex(ValueError, "on the same GPU device"):
bsr_dense_mm(lhs.to("cuda:0"), rhs.to("cuda:1"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep all mutli-GPU tests separate (and decorated with @requiresMultiGPU or something like that)

check(
not n % row_block,
f"bsr_dense_mm(): dense.size(-1) == {n} should be divisible by "
f"blocksize[0] == {row_block}.",
)

def is_power_of_two(v):
return not (v & (v - 1))
Copy link
Contributor

@malfet malfet May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (I'm not sure how not is defined for integers in Python, perhaps you can send a link

Suggested change
return not (v & (v - 1))
return v & (v - 1) == 0

Copy link
Collaborator Author

@nikitaved nikitaved May 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://docs.python.org/3/reference/expressions.html#not:

"In the context of Boolean operations, and also when expressions are used by control flow statements, the following values are interpreted as false: False, None, numeric zero of all types, and empty strings and containers (including strings, tuples, lists, dictionaries, sets and frozensets). All other values are interpreted as true. User-defined objects can customize their truth value by providing a bool() method."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point here, is that, in my mind, mixing logical and and boolean not hurts readability, on the other hand, it's up to an author/maintainer of the codebase. I.e. as long as @cpuhrsch is fine reading that, I'm ok as well.

torch/sparse/_triton_ops.py Show resolved Hide resolved
This PR improves test coverage for `bsr_dense_mm` by:
- ~~enabling correctness tests for `float32`~~.
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
This PR improves test coverage for `bsr_dense_mm` by:
- ~~enabling correctness tests for `float32`~~.
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
This PR improves test coverage for `bsr_dense_mm` by:
- ~~enabling correctness tests for `float32`~~.
- extending and testing input correctness checks.




cc alexsamardzic pearu cpuhrsch amjames bhosmer

[ghstack-poisoned]
@nikitaved
Copy link
Collaborator Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@facebook-github-bot facebook-github-bot deleted the gh/nikitaved/40/head branch June 8, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: sparse Related to torch.sparse open source release notes: sparse release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants