Skip to content
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.

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

[tensor] Make isEqual work correctly with NAN #2443

Merged
merged 1 commit into from Feb 27, 2019

Conversation

Projects
None yet
3 participants
@bertmaher
Copy link
Contributor

bertmaher commented Feb 25, 2019

Description: We were erroneously reporting NAN as equal to everything instead of not-equal.
Testing: New unit tests.
Documentation: n/a

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

bertmaher commented Feb 25, 2019

This is still arguably incorrect for INF, since INF vs INF should be true (IEEE speaking), but this method will report false now (since INF - INF => NAN). I wonder if there's a fix that isn't super complicated, but this PR is at least better than treating NANs as widcards :-/

@jackm321
Copy link
Contributor

jackm321 left a comment

LGTM

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

bertmaher commented Feb 26, 2019

The compiler saw through my clever negation trick, and optimized it away in our Release build (because of -ffast-math, it assumes NaN never happens). So let's go with an explicit std::isnan check, which seems to be OK in release mode.

(Oddly, if I write a standalone function that calls std::isnan and look at the disassembly, it always returns false. So, I'm not really sure how much confidence I should have in std::isnan either)

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

bertmaher commented Feb 26, 2019

Compiler 2, Bert 0. Going to revisit this problem tomorrow.

@jackm321

This comment has been minimized.

Copy link
Contributor

jackm321 commented Feb 26, 2019

Lol I'm also going 0/2 with the accepts, I should have looked at the test output first.

@jackm321

This comment has been minimized.

Copy link
Contributor

jackm321 commented Feb 26, 2019

Just messed around in godbolt for a bit but didn't find much, will be interesting if there's a way around this one.

@bertmaher bertmaher force-pushed the bertmaher:isequal branch from 95e78b9 to cb8826b Feb 27, 2019

@bertmaher

This comment has been minimized.

Copy link
Contributor Author

bertmaher commented Feb 27, 2019

After some fiddling with godbolt I just don't think it's sane to try to handle NAN when -ffinite-math-only (implied by -ffast-math) is in effect. Even if something happens to work, the compiler doesn't guarantee that it will continue working. So, let's disable finite-math-only.

@bertmaher bertmaher force-pushed the bertmaher:isequal branch from 0c5210d to e301f7f Feb 27, 2019

@bertmaher bertmaher merged commit 1c8eadb into pytorch:master Feb 27, 2019

6 checks passed

ci/circleci: ASAN Your tests passed on CircleCI!
Details
ci/circleci: DEBUG Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_WITH_EXPENSIVE_TESTS Your tests passed on CircleCI!
Details
ci/circleci: SHARED Your tests passed on CircleCI!
Details
ci/circleci: TSAN Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bertmaher bertmaher deleted the bertmaher:isequal branch Feb 27, 2019

vuzelac-cadence added a commit to Cadence-TIP-AI/glow that referenced this pull request Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.