Skip to content

Conversation

@stas00
Copy link
Contributor

@stas00 stas00 commented Aug 21, 2021

pytorch-1.10-to-be dropped the non-public API torch.testing.assert_equal, see: https://pytorch.org/docs/stable/testing.html

So this is a follow up to: #47 to make the tests work with pt-nightly.

I used

torch.testing.assert_close(liglu(self.x), expected, rtol=0.0, atol=0.0)

to replace the function that was removed.

Perhaps we can add a wrapper for torch_assert_equal in testing_utils.py instead?

@jaketae

@jaketae
Copy link
Member

jaketae commented Aug 21, 2021

I wasn't aware of the API changes in the torch nightly builds, thanks for keeping me up-to-date on this.

Yes, I agree that a wrapper would be cleaner, although the PR in its current form would also work. So if I'm understanding this correctly, we would have in testing_utils.py something like

# assume imports
def torch_assert_equal(actual, expected, rtol=0.0, atol=0.0):
    return torch.testing.assert_close(actual, expected, rtol=rtol, atol=atol)

Depending on how you'd like to proceed logistically, we can either implement the wrapper in this PR, or we can merge this in first, and I'll open a new PR that adds the wrapper to testing_utils.py. Do you have a preference?

@stas00
Copy link
Contributor Author

stas00 commented Aug 21, 2021

Almost, as this is equal, it'd just be:

def torch_assert_equal(actual, expected):
    torch.testing.assert_close(actual, expected, rtol=0.0, atol=0.0)

or can even use partial:

from functools import partial
torch_assert_equal = partial(torch.testing.assert_close, rtol=0.0, atol=0.0)

but this one is harder to document, so the former it is.

I pushed the change to use the wrapper.

@jaketae
Copy link
Member

jaketae commented Aug 21, 2021

Looks good to me! Agreed that the former would suffice. Thanks for taking care of this.

@jaketae jaketae merged commit a96e2ab into main Aug 21, 2021
@stas00 stas00 deleted the pt-110-fixes branch August 21, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants