Skip to content

Conversation

aocsa
Copy link
Contributor

@aocsa aocsa commented Apr 19, 2021

Stack from ghstack:

Fixes for gh-56371 and gh-56369

Differential Revision: D27913212

Note:
The following requests about testing listed here

#50937 (comment) - see modern COO testing
#50937 (comment) - avoid using numpy
#50937 (comment) - inefficient CSR samples

are being solved in this PR

minor fix

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Apr 19, 2021

💊 CI failures summary and remediations

As of commit 2cd9c73 (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.

Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

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

Hey @aocsa! Overall this looks pretty good; I have a couple suggestions and a question, but nothing major

@aocsa aocsa changed the title Modernize test-suite in sparse tensor CSR Modernize test-suite in sparse tensors Apr 22, 2021
Fixes for gh-56371 and gh-56369

Differential Revision: [D27913212](https://our.internmc.facebook.com/intern/diff/D27913212)

Note: 
The following requests about testing listed here https://pearu.github.io/csr_tensor_support.html

#50937 (comment) - see modern COO testing
#50937 (comment) - don’t set default_dtype_type
#50937 (comment) - avoid using numpy
#50937 (comment) - inefficient CSR samples

are being solved in this PR

[ghstack-poisoned]
@aocsa aocsa changed the title Modernize test-suite in sparse tensors Modernize test-suite in sparse tensors CSR Apr 22, 2021
@aocsa aocsa changed the title Modernize test-suite in sparse tensors CSR Modernize test-suite in sparse tensors Apr 22, 2021
@aocsa aocsa mentioned this pull request Apr 22, 2021
Fixes for gh-56371 and gh-56369

Differential Revision: [D27913212](https://our.internmc.facebook.com/intern/diff/D27913212)

Note: 
The following requests about testing listed here https://pearu.github.io/csr_tensor_support.html

#50937 (comment) - see modern COO testing
#50937 (comment) - avoid using numpy
#50937 (comment) - inefficient CSR samples

are being solved in this PR

[ghstack-poisoned]
Copy link
Collaborator

@pearu pearu left a comment

Choose a reason for hiding this comment

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

Thanks @aocsa , this looks good.

I have several nits but most importantly, I suggest making test_sparse_csr_constructor method more thorough, in particular, ensure that all supported dtypes are covered as well as checking that the constructed CSR tensor have the expected dtype, device, and indices/values.

We may also want to extend these tests to other inputs such as lists and objects implementing array interface but atm this blocked by #56687 and #54187 .

Fixes for gh-56371 and gh-56369

Differential Revision: [D27913212](https://our.internmc.facebook.com/intern/diff/D27913212)

Note: 
The following requests about testing listed [here](https://pearu.github.io/csr_tensor_support.html) 

#50937 (comment) - see modern COO testing
#50937 (comment) - avoid using numpy
#50937 (comment) - inefficient CSR samples

are being solved in this PR

[ghstack-poisoned]
@aocsa
Copy link
Contributor Author

aocsa commented Apr 23, 2021

Thanks @aocsa , this looks good.

I have several nits but most importantly, I suggest making test_sparse_csr_constructor method more thorough, in particular, ensure that all supported dtypes are covered as well as checking that the constructed CSR tensor have the expected dtype, device, and indices/values.

We may also want to extend these tests to other inputs such as lists and objects implementing array interface but atm this blocked by #56687 and #54187 .

Thanks for the feedback @pearu, I addressed the comments and rebased this PR on top of the latest changes, polished a few things, like better CSR sample input generator see #56392 (comment) and added some tests, specifically related to factory tests. I think it is ready for final review and merging, let me know what you think. cc @mruberry

Fixes for gh-56371 and gh-56369

Differential Revision: [D27913212](https://our.internmc.facebook.com/intern/diff/D27913212)

Note: 
The following requests about testing listed [here](https://pearu.github.io/csr_tensor_support.html) 

#50937 (comment) - see modern COO testing
#50937 (comment) - avoid using numpy
#50937 (comment) - inefficient CSR samples

are being solved in this PR

[ghstack-poisoned]
Copy link
Collaborator

@pearu pearu left a comment

Choose a reason for hiding this comment

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

Thanks @aocsa ! LGTM, however, there is a bug in random_sparse_csr. Once this is fixed, I'll approve.

Fixes for gh-56371 and gh-56369

Differential Revision: [D27913212](https://our.internmc.facebook.com/intern/diff/D27913212)

Note: 
The following requests about testing listed [here](https://pearu.github.io/csr_tensor_support.html) 

#50937 (comment) - see modern COO testing
#50937 (comment) - avoid using numpy
#50937 (comment) - inefficient CSR samples

are being solved in this PR

[ghstack-poisoned]
Copy link
Collaborator

@pearu pearu left a comment

Choose a reason for hiding this comment

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

Thanks, @aocsa for the fixes.

I'll approve this PR despite the wrong crow_indices[-1] <= nnz check, see #56392 (comment) .
We'll need to resolve this incorrectness in the context of #56856 because the corresponding fix likely requires changes to the CSR construction algorithm and hence is OT for this PR.

@mruberry I think this PR is ready for merge (assuming that CI is fine with it). The PR has a high priority because other CSR-related PRs rely on this testing framework.

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in 18c89a9.

@facebook-github-bot facebook-github-bot deleted the gh/aocsa/6/head branch May 1, 2021 14:16
aocsa added a commit to Quansight/pytorch that referenced this pull request May 3, 2021
Summary:
Pull Request resolved: pytorch#56392

Fixes for pytorchgh-56371 and pytorchgh-56369

Test Plan: Imported from OSS

Reviewed By: H-Huang

Differential Revision: D27913212

Pulled By: mruberry

fbshipit-source-id: 2c78fe9fa4b6c6b566d9eb01f71e6016d672a545
crcrpar pushed a commit to crcrpar/pytorch that referenced this pull request May 7, 2021
Summary:
Pull Request resolved: pytorch#56392

Fixes for pytorchgh-56371 and pytorchgh-56369

Test Plan: Imported from OSS

Reviewed By: H-Huang

Differential Revision: D27913212

Pulled By: mruberry

fbshipit-source-id: 2c78fe9fa4b6c6b566d9eb01f71e6016d672a545
krshrimali pushed a commit to krshrimali/pytorch that referenced this pull request May 19, 2021
Summary:
Pull Request resolved: pytorch#56392

Fixes for pytorchgh-56371 and pytorchgh-56369

Test Plan: Imported from OSS

Reviewed By: H-Huang

Differential Revision: D27913212

Pulled By: mruberry

fbshipit-source-id: 2c78fe9fa4b6c6b566d9eb01f71e6016d672a545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants