Fix inverted is_all_nan / is_any_nan / is_all_infinite / is_any_infinite - #1614
Open
youdie006 wants to merge 1 commit into
Open
Fix inverted is_all_nan / is_any_nan / is_all_infinite / is_any_infinite#1614youdie006 wants to merge 1 commit into
youdie006 wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
The boolean_ops! macro in src/numeric/impl_float_maths.rs had an inverted $all predicate: Zip::from(self).all(|&elt| !elt.$func()) tests "all elements are NOT NaN", the opposite of the documented "true if all elements are NaN". $any was defined as !self.$all(), so the double negation made is_any_* appear correct while is_all_* returned true for an all-finite array. Correct both predicates together (fixing only $all would break the is_any_* methods that ride on the double negation): $all -> Zip::from(self).all(|&elt| elt.$func()) $any -> Zip::from(self).any(|&elt| elt.$func()) Empty-array semantics are preserved (all -> true, any -> false). Fixes rust-ndarray#1612
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1612.
Root cause
The boolean_ops! macro in src/numeric/impl_float_maths.rs had an inverted $all predicate: Zip::from(self).all(|&elt| !elt.$func()) tests "all elements are NOT NaN", the opposite of the documented "true if all elements are NaN". $any was defined as !self.$all(), so the double negation made is_any_* appear correct while is_all_* returned true for an all-finite array.
Fix
Both predicates are corrected together (fixing only $all would break the is_any_* methods that currently ride on the double negation):
This fixes is_all_nan, is_any_nan, is_all_infinite, and is_any_infinite in one change. Empty-array semantics are preserved (all -> true, any -> false).
@dbranford correctly identified the $all flip and raised the is_any question in the issue thread; this PR applies both corrections and adds regression coverage.
Tests
New tests/nan_all_repro.rs covers both the NaN and infinite families with f64: all-finite (is_all_* and is_any_* false), all-nan/all-inf (is_all_* and is_any_* true), and mixed (is_all_* false, is_any_* true).
Red-green verified: with the fix reverted, the first assertion (!all_finite.is_all_nan()) panics; with the fix, both tests pass. cargo fmt (nightly, per repo rustfmt.toml), cargo clippy (no new warnings), and the existing tests/numeric.rs suite all pass.
AI-assisted: this change was prepared with the help of an AI coding assistant and reviewed by me before submission.