KEMBAR78
Improve error messages of `torch.testing.assert_close` for sparse inputs by pmeier · Pull Request #61583 · pytorch/pytorch · GitHub
Skip to content

Improve error messages of torch.testing.assert_close for sparse inputs #61583

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Improve error messages of torch.testing.assert_close for sparse inputs #61583

wants to merge 1 commit into from

Conversation

@pmeier
Copy link
Collaborator

@pmeier pmeier commented Jul 13, 2021 •
edited
Loading

This utilizes the feature introduced in #60091 to modify the header of the error message.

Before:

AssertionError: Tensor-likes are not equal!

Mismatched elements: 1 / 2 (50.0%)
Greatest absolute difference: 1 at index 1
Greatest relative difference: 0.3333333432674408 at index 1

The failure occurred for the values.

After:

AssertionError: Sparse COO values of tensor-likes are not equal!

Mismatched elements: 1 / 2 (50.0%)
Greatest absolute difference: 1 at index 1
Greatest relative difference: 0.3333333432674408 at index 1

@pmeier pmeier added module: sparse Related to torch.sparse module: testing Issues related to the torch.testing module (not tests) labels Jul 13, 2021
@pmeier pmeier requested a review from mruberry July 13, 2021 16:04
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jul 13, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 578dd5d (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.

Click here to manually regenerate this comment.

@mruberry mruberry requested a review from cpuhrsch July 15, 2021 08:53
@mruberry
Copy link
Collaborator

@cpuhrsch @IvanYashchuk - want to take a look at this?

@mruberry mruberry requested a review from IvanYashchuk July 15, 2021 08:53
IvanYashchuk
IvanYashchuk approved these changes Jul 15, 2021
View reviewed changes
Copy link
Collaborator

@IvanYashchuk IvanYashchuk left a comment

Choose a reason for hiding this comment

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

New error messages look great!

A few questions on comparison functions:

  1. _check_sparse_coo_members_individually checks uncoalesced values and indices. There could be a situation when different uncoalesced sparse COO tensors would be equal when coalesced (sparse.coalesce()). Maybe it would be useful to have a possibility to test that?
  2. _check_sparse_csr_members_individually doesn't check _nnz(), while comparison for COO tensors does that, should it be added here? If all further indices and values checks pass, nnz should not be different though.

@pmeier
Copy link
Collaborator Author

pmeier commented Jul 15, 2021

@IvanYashchuk

  1. _check_sparse_coo_members_individually checks uncoalesced values and indices. There could be a situation when different uncoalesced sparse COO tensors would be equal when coalesced (sparse.coalesce()). Maybe it would be useful to have a possibility to test that?

Not sure I fully understand. We have a check_is_coalesced flag that by default checks if both tensors are either coalesced or non-coalesced:

pytorch/torch/testing/_asserts.py

Lines 238 to 241 in 9496521

elif actual.layout == torch.sparse_coo and check_is_coalesced and actual.is_coalesced() != expected.is_coalesced():
return _TestingErrorMeta(
AssertionError, msg_fmtstr.format("is_coalesced()", actual.is_coalesced(), expected.is_coalesced())
)

If you disable this check, both tensors will be coalesce'd before the comparison:

pytorch/torch/testing/_asserts.py

Lines 283 to 285 in 9496521

if actual.is_sparse and actual.is_coalesced() != expected.is_coalesced():
actual = actual.coalesce()
expected = expected.coalesce()

  1. _check_sparse_csr_members_individually doesn't check _nnz(), while comparison for COO tensors does that, should it be added here? If all further indices and values checks pass, nnz should not be different though.

Discussion for COO in #58844 (comment). If it is useful, I'm fine with adding it to CSR as well. cc @pearu

@IvanYashchuk
Copy link
Collaborator

We have a check_is_coalesced flag that by default checks if both tensors are either coalesced or non-coalesced

Thanks! I didn't know about it.

@mruberry
Copy link
Collaborator

@cpuhrsch did you want to take a look at this?

@facebook-github-bot
Copy link
Contributor

@cpuhrsch has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@cpuhrsch merged this pull request in f16c73b.

@pmeier pmeier deleted the sparse-error-msgs branch August 19, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuhrsch cpuhrsch cpuhrsch approved these changes

@IvanYashchuk IvanYashchuk IvanYashchuk approved these changes

@mruberry mruberry Awaiting requested review from mruberry

Assignees

No one assigned

Labels

cla signed Merged module: sparse Related to torch.sparse module: testing Issues related to the torch.testing module (not tests) open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pmeier @facebook-github-bot @mruberry @IvanYashchuk @cpuhrsch @pytorchbot