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.

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

Made nan's quiet to hide FPEs #4180

Merged
merged 1 commit into from
Jan 15, 2019
Merged

Made nan's quiet to hide FPEs #4180

merged 1 commit into from
Jan 15, 2019

Conversation

bathmatt
Copy link
Contributor

make nan's quiet

@bartlettroscoe bartlettroscoe added the stage: in progress Work on the issue has started label Jan 12, 2019
@bathmatt bathmatt added the AT: AUTOMERGE Causes the PR autotester to automatically merge the PR branch once approvals are completed label Jan 12, 2019
@bathmatt bathmatt mentioned this pull request Jan 12, 2019
Closed
@mhoemmen mhoemmen added the pkg: Teuchos Issues primarily dealing with the Teuchos Package label Jan 13, 2019
@mhoemmen
Copy link
Contributor

This also brings Teuchos::ScalarTraits' behavior in line with that of Kokkos::ArithTraits. See kokkos/kokkos-kernels#35.

@bathmatt
Copy link
Contributor Author

I still don't understand the point of using a quiet_nan vs a signaling nan, if the point of setting something to NaN is that you catch errors, wouldn't you want to catch those errors when they start? Quiet nan seems useful for a flag, but since you can do math and the quiet bit propagates to all the other nans it seems somewhat pointless.

I do understand that you can throw nan's in algorithms in a harmless way, then you might wanna use quiet, but I think that caching the FPE handling flags, handling the case where you go NaN and then resetting them after is a better way to handle this case.

@krcb
Copy link
Contributor

krcb commented Jan 14, 2019

I agree with @bathmatt. Switching to the signaling NaN allowed an application to catch a range of errors in a Trilinos package that were generating NaNs quietly before. It seems that when an algorithm wants to use NaNs in a quiet way, this should be done explicitly. However, it appears though that the Teuchos::ScalarTraits::nan() implementation is being used both when an algorithm wants a harmless NaN and to indicate there's something wrong (e.g. in the Teuchos::ScalarTraits::squareroot() implementation). How is downstream application supposed to distinguish between these two cases?

@mhoemmen
Copy link
Contributor

How is downstream application supposed to distinguish between these two cases?

There's no way to distinguish. You just have to know that an algorithm might generate harmless NaNs, and turn off signaling while that algorithm runs.

I like IEEE 754 and I want users to have the option to enable signaling NaNs, but I think changing Teuchos (nearly the most upstream package in Trilinos) without a lot of application testing was premature.

@krcb
Copy link
Contributor

krcb commented Jan 14, 2019

@mhoemmen Would a possible compromise be to add a compile time option to switch between quiet and signaling NaNs? This option would default to the quiet NaN case and and then an application could decide whether or not to turn it on.

@mhoemmen
Copy link
Contributor

Would a possible compromise be to add a compile time option to switch between quiet and signaling NaNs?

I'm not a fan of adding Still More Compile-Time Options. No one will ever test the signaling one, then someone will try it and it won't compile, exactly at the time when you needed it to.

If you decide to do it, note that the option would need to control both Teuchos::ScalarTraits and Kokkos::ArithTraits. Right now, you've only been looking at ScalarTraits.

@trilinos-autotester
Copy link
Contributor

Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request.

@trilinos-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: Trilinos_pullrequest_gcc_4.8.4

  • Build Num: 2235
  • Status: STARTED

Jenkins Parameters

Parameter Name Value
COMPILER_MODULE sems-gcc/4.8.4
JENKINS_BUILD_TYPE Release
JENKINS_COMM_TYPE MPI
JENKINS_DO_COMPLEX OFF
JENKINS_JOB_TYPE Experimental
MPI_MODULE sems-openmpi/1.8.7
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Build Information

Test Name: Trilinos_pullrequest_intel_17.0.1

  • Build Num: 2038
  • Status: STARTED

Jenkins Parameters

Parameter Name Value
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Build Information

Test Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL

  • Build Num: 525
  • Status: STARTED

Jenkins Parameters

Parameter Name Value
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Build Information

Test Name: Trilinos_pullrequest_gcc_7.2.0

  • Build Num: 136
  • Status: STARTED

Jenkins Parameters

Parameter Name Value
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Using Repos:

Repo: TRILINOS (trilinos/Trilinos)
  • Branch: quiet-nan
  • SHA: 108c7f8
  • Mode: TEST_REPO

Pull Request Author: bathmatt

@trilinos-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: Trilinos_pullrequest_gcc_4.8.4

  • Build Num: 2235
  • Status: PASSED

Jenkins Parameters

Parameter Name Value
COMPILER_MODULE sems-gcc/4.8.4
JENKINS_BUILD_TYPE Release
JENKINS_COMM_TYPE MPI
JENKINS_DO_COMPLEX OFF
JENKINS_JOB_TYPE Experimental
MPI_MODULE sems-openmpi/1.8.7
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Build Information

Test Name: Trilinos_pullrequest_intel_17.0.1

  • Build Num: 2038
  • Status: PASSED

Jenkins Parameters

Parameter Name Value
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Build Information

Test Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL

  • Build Num: 525
  • Status: PASSED

Jenkins Parameters

Parameter Name Value
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304

Build Information

Test Name: Trilinos_pullrequest_gcc_7.2.0

  • Build Num: 136
  • Status: PASSED

Jenkins Parameters

Parameter Name Value
PULLREQUESTNUM 4180
TEST_REPO_ALIAS TRILINOS
TRILINOS_SOURCE_BRANCH quiet-nan
TRILINOS_SOURCE_REPO https://github.com/trilinos/Trilinos
TRILINOS_SOURCE_SHA 108c7f8
TRILINOS_TARGET_BRANCH develop
TRILINOS_TARGET_REPO https://github.com/trilinos/Trilinos
TRILINOS_TARGET_SHA 47dc304


CDash Test Results for PR# 4180.

@trilinos-autotester
Copy link
Contributor

Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ mhoemmen ]!

@trilinos-autotester
Copy link
Contributor

Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged

@trilinos-autotester trilinos-autotester merged commit fa493f2 into develop Jan 15, 2019
@trilinos-autotester
Copy link
Contributor

Merge on Pull Request# 4180: IS A SUCCESS - Pull Request successfully merged

@trilinos-autotester trilinos-autotester removed the AT: AUTOMERGE Causes the PR autotester to automatically merge the PR branch once approvals are completed label Jan 15, 2019
@bartlettroscoe bartlettroscoe removed the stage: in progress Work on the issue has started label Jan 15, 2019
@jhux2 jhux2 deleted the quiet-nan branch August 14, 2019 17:50
@trilinos-autotester trilinos-autotester mentioned this pull request Feb 17, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhoemmen mhoemmen mhoemmen approved these changes

Assignees

@mhoemmen mhoemmen

Labels
pkg: Teuchos Issues primarily dealing with the Teuchos Package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bathmatt @mhoemmen @krcb @trilinos-autotester @bartlettroscoe