KEMBAR78
make assert_equal an example how to partial `torch.testing.assert_close` by pmeier · Pull Request #58918 · pytorch/pytorch · GitHub
Skip to content

Conversation

@pmeier
Copy link
Collaborator

@pmeier pmeier commented May 25, 2021

Stack from ghstack:

Instead of a distinct torch.testing.assert_close and torch.testing.assert_equal, this makes torch.testing.assert_equal a special case of torch.testing.assert_close for rtol=atol=0. In this case the closeness definition abs(actual - expected) <= atol + rtol * abs(expected) boils down to abs(actual - expected) <= 0. Since abs(x) can never be <0, this is equivalent to abs(a - b) == 0 and this again boils down to a == b.

Following #58918 (comment) and some offline discussions, we opted to use assert_equal as an example how to partial it.

This makes maintaing the module a lot easier, because we don't need to keep two functions in sync.

Differential Revision: D29259404

Instead of a distinct `torch.testing.assert_close` and
`torch.testing.assert_equal`, this makes `torch.testing.assert_equal` a
special case of `torch.testing.assert_close` for `rtol=atol=0`. In this
case the closeness definition `abs(actual - expected) <= atol + rtol *
abs(expected)` boils down to `abs(actual - expected) <= 0`. Since
`abs(x)` can never be `<0`, this is equivalent to `abs(a - b) == 0` and
this again boils down to `a == b`.

This makes maintaing the module a lot easier, because we don't need to
keep two functions in sync.

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

facebook-github-bot commented May 25, 2021

💊 CI failures summary and remediations

As of commit 0f9e7b8 (more details on the Dr. CI page and at hud.pytorch.org/pr/58918):


💚 💚 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.

@pmeier
Copy link
Collaborator Author

pmeier commented Jun 14, 2021

Instead of defining assert_equal ourselves, we could also use it as a good example on how to partial from torch.testing.assert_close. Thoughts @mruberry?

@pmeier pmeier requested a review from mruberry June 14, 2021 12:25
@mruberry
Copy link
Collaborator

Is there logic like

somewhere, where this will short-circuit to a bitwise equivalence test?

@mruberry
Copy link
Collaborator

Lint failures are real

@pmeier
Copy link
Collaborator Author

pmeier commented Jun 16, 2021

Is there logic like

somewhere, where this will short-circuit to a bitwise equivalence test?

Nope, in the current state we will always use torch.isclose for comparison, i.e. checking if abs(actual - expected) == 0 holds. I'm not sure if there are implementation details that would make this not equivalent to ==.

We could also use an equal check if rtol == 0 and atol == 0, but this is means more implementation since we need to handle equal_nan separately. Plus the differences would need to be documented. So if there is no apparent use case, I would leave it as is.

Instead of a distinct `torch.testing.assert_close` and
`torch.testing.assert_equal`, this makes `torch.testing.assert_equal` a
special case of `torch.testing.assert_close` for `rtol=atol=0`. In this
case the closeness definition `abs(actual - expected) <= atol + rtol *
abs(expected)` boils down to `abs(actual - expected) <= 0`. Since
`abs(x)` can never be `<0`, this is equivalent to `abs(a - b) == 0` and
this again boils down to `a == b`.

This makes maintaing the module a lot easier, because we don't need to
keep two functions in sync.

[ghstack-poisoned]
….assert_close`"

~Instead of a distinct `torch.testing.assert_close` and `torch.testing.assert_equal`, this makes `torch.testing.assert_equal` a special case of `torch.testing.assert_close` for `rtol=atol=0`. In this case the closeness definition `abs(actual - expected) <= atol + rtol * abs(expected)` boils down to `abs(actual - expected) <= 0`. Since `abs(x)` can never be `<0`, this is equivalent to `abs(a - b) == 0` and this again boils down to `a == b`.~

Following #58918 (comment) and some offline discussions, we opted to use `assert_equal` as an example how to `partial` it.

This makes maintaing the module a lot easier, because we don't need to keep two functions in sync.

[ghstack-poisoned]
@pmeier pmeier changed the title make assert_equal a special case of assert_close make assert_equal an example how to partial torch.testing.assert_close Jun 17, 2021
pmeier added a commit that referenced this pull request Jun 17, 2021
…l `torch.testing.assert_close`"

~Instead of a distinct `torch.testing.assert_close` and `torch.testing.assert_equal`, this makes `torch.testing.assert_equal` a special case of `torch.testing.assert_close` for `rtol=atol=0`. In this case the closeness definition `abs(actual - expected) <= atol + rtol * abs(expected)` boils down to `abs(actual - expected) <= 0`. Since `abs(x)` can never be `<0`, this is equivalent to `abs(a - b) == 0` and this again boils down to `a == b`.~

Following #58918 (comment) and some offline discussions, we opted to use `assert_equal` as an example how to `partial` it.

This makes maintaing the module a lot easier, because we don't need to keep two functions in sync.

[ghstack-poisoned]
Comment on lines 648 to 652
AssertionError: Tensors are not close!
Mismatched elements: 1 / 1 (100.0%)
Greatest absolute difference: 8.999999703829253e-10 at 0 (up to 0 allowed)
Greatest relative difference: 8.999999583666371 at 0 (up to 0 allowed)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will make much more sense after #60091 from later in this stack is in. The error will read:

AssertionError: Scalars are not equal!

Absolute difference: 8.999999703829253e-10
Relative difference: 8.999999583666371

….assert_close`"

~Instead of a distinct `torch.testing.assert_close` and `torch.testing.assert_equal`, this makes `torch.testing.assert_equal` a special case of `torch.testing.assert_close` for `rtol=atol=0`. In this case the closeness definition `abs(actual - expected) <= atol + rtol * abs(expected)` boils down to `abs(actual - expected) <= 0`. Since `abs(x)` can never be `<0`, this is equivalent to `abs(a - b) == 0` and this again boils down to `a == b`.~

Following #58918 (comment) and some offline discussions, we opted to use `assert_equal` as an example how to `partial` it.

This makes maintaing the module a lot easier, because we don't need to keep two functions in sync.

[ghstack-poisoned]
pmeier added a commit that referenced this pull request Jun 18, 2021
…l `torch.testing.assert_close`"

~Instead of a distinct `torch.testing.assert_close` and `torch.testing.assert_equal`, this makes `torch.testing.assert_equal` a special case of `torch.testing.assert_close` for `rtol=atol=0`. In this case the closeness definition `abs(actual - expected) <= atol + rtol * abs(expected)` boils down to `abs(actual - expected) <= 0`. Since `abs(x)` can never be `<0`, this is equivalent to `abs(a - b) == 0` and this again boils down to `a == b`.~

Following #58918 (comment) and some offline discussions, we opted to use `assert_equal` as an example how to `partial` it.

This makes maintaing the module a lot easier, because we don't need to keep two functions in sync.

[ghstack-poisoned]
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.

Cool

@mruberry
Copy link
Collaborator

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

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in 14b0191.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants