Skip to content

Conversation

NicolasHug
Copy link
Member

part of #3865

Copy link
Member Author

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Minor comment but it's more about assert_close than about the proposed changes in this PR, LGTM

Comment on lines +89 to +90
torch.testing.assert_close(dboxes[0], dboxes_output, rtol=1e-5, atol=1e-8)
torch.testing.assert_close(dboxes[1], dboxes_output, rtol=1e-5, atol=1e-8)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here it's a bit of a shame IMHO that we're forced to specify atol as well. The default for atol is 1e-5 and

        torch.testing.assert_close(dboxes[0], dboxes_output, rtol=1e-5, atol=1e-5)
        torch.testing.assert_close(dboxes[1], dboxes_output, rtol=1e-5, atol=1e-5)

passes just fine, so ideally we could just write

        torch.testing.assert_close(dboxes[0], dboxes_output, rtol=1e-5)
        torch.testing.assert_close(dboxes[1], dboxes_output, rtol=1e-5)

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Stamping

@NicolasHug NicolasHug merged commit 86d4541 into pytorch:master May 21, 2021
facebook-github-bot pushed a commit that referenced this pull request May 25, 2021
Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Reviewed By: vincentqb, cpuhrsch

Differential Revision: D28679964

fbshipit-source-id: c53eb63031a4b965e870f3334d749e4d9d41ddad
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.

4 participants