Skip to content

Remove NaN and Inf checks if __FINITE_MATH_ONLY__ is defined - #35

Draft
garrison wants to merge 1 commit into
mainfrom
finite-math-only
Draft

Remove NaN and Inf checks if __FINITE_MATH_ONLY__ is defined#35
garrison wants to merge 1 commit into
mainfrom
finite-math-only

Conversation

@garrison

@garrison garrison commented Mar 21, 2026

Copy link
Copy Markdown
Member

This is motivated by qiskit-community/fulqrum#3. If finite-only math is used, then these checks are unnecessary, and we can remove them to spare the user some warnings.

However, I am hesitant to go out of my way much to support compilers that have been instructed to break IEEE 754 semantics, so I am still considering whether it makes sense to merge this.

  • Should we add tests in CI with fast-math enabled? This would imply we support this explicitly, which may be a bad idea.
  • Should we instead consider hard-failing if __FAST_MATH__ is defined?

@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 23369575181

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 19 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+6.4%) to 57.387%

Files with Coverage Reduction New Missed Lines %
include/qiskit/addon/sqd/internal/sample-without-replacement.hpp 1 67.8%
include/qiskit/addon/sqd/configuration_recovery.hpp 2 80.0%
include/qiskit/addon/sqd/subsampling.hpp 4 64.0%
test/test_configuration_recovery.cpp 5 50.78%
include/qiskit/addon/sqd/fermion.hpp 7 53.57%
Totals Coverage Status
Change from base Build 20271294895: 6.4%
Covered Lines: 377
Relevant Lines: 408

💛 - Coveralls

@garrison
garrison marked this pull request as draft March 21, 2026 02:02
@garrison

Copy link
Copy Markdown
Member Author

After further thought, I think we should disable these checks if __FINITE_MATH_ONLY__ is defined, as this PR does. We should also display a warning if a user compiles with __FAST_MATH__ defined, and we should document that we do not support the use of fast math.

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.

2 participants