Skip to content

Commit

Permalink
revert: reduce guarantees on signed zero/nan (#11564)
Browse files Browse the repository at this point in the history
  • Loading branch information
orlp authored Oct 6, 2023
1 parent 7aeccac commit 3b40c7a
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 34 deletions.
23 changes: 8 additions & 15 deletions crates/nano-arrow/src/compute/arithmetics/basic/div.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! Definition of basic div operations with primitive arrays
use std::ops::{Add, Div};
use std::ops::Div;

use num_traits::{CheckedDiv, NumCast};
use strength_reduce::{
Expand Down Expand Up @@ -29,18 +29,14 @@ use crate::datatypes::PrimitiveType;
/// ```
pub fn div<T>(lhs: &PrimitiveArray<T>, rhs: &PrimitiveArray<T>) -> PrimitiveArray<T>
where
T: NativeArithmetics + Add<Output = T> + Div<Output = T>,
T: NativeArithmetics + Div<Output = T>,
{
// Adding zero to divisor ensures x/0 becomes +infinity, ignoring
// the sign of the zero.
if rhs.null_count() == 0 {
binary(lhs, rhs, lhs.data_type().clone(), |a, b| {
a / (b + T::zeroed())
})
binary(lhs, rhs, lhs.data_type().clone(), |a, b| a / b)
} else {
check_same_len(lhs, rhs).unwrap();
let values = lhs.iter().zip(rhs.iter()).map(|(l, r)| match (l, r) {
(Some(l), Some(r)) => Some(*l / (*r + T::zeroed())),
(Some(l), Some(r)) => Some(*l / *r),
_ => None,
});

Expand All @@ -65,19 +61,16 @@ where
/// ```
pub fn checked_div<T>(lhs: &PrimitiveArray<T>, rhs: &PrimitiveArray<T>) -> PrimitiveArray<T>
where
T: NativeArithmetics + Add<Output = T> + CheckedDiv<Output = T>,
T: NativeArithmetics + CheckedDiv<Output = T>,
{
// Adding zero to divisor ensures x/0 becomes +infinity, ignoring
// the sign of the zero.
let op = move |a: T, b: T| a.checked_div(&(b + T::zeroed()));

let op = move |a: T, b: T| a.checked_div(&b);
binary_checked(lhs, rhs, lhs.data_type().clone(), op)
}

// Implementation of ArrayDiv trait for PrimitiveArrays
impl<T> ArrayDiv<PrimitiveArray<T>> for PrimitiveArray<T>
where
T: NativeArithmetics + Add<Output = T> + Div<Output = T>,
T: NativeArithmetics + Div<Output = T>,
{
fn div(&self, rhs: &PrimitiveArray<T>) -> Self {
div(self, rhs)
Expand All @@ -87,7 +80,7 @@ where
// Implementation of ArrayCheckedDiv trait for PrimitiveArrays
impl<T> ArrayCheckedDiv<PrimitiveArray<T>> for PrimitiveArray<T>
where
T: NativeArithmetics + Add<Output = T> + CheckedDiv<Output = T>,
T: NativeArithmetics + CheckedDiv<Output = T>,
{
fn checked_div(&self, rhs: &PrimitiveArray<T>) -> Self {
checked_div(self, rhs)
Expand Down
13 changes: 3 additions & 10 deletions crates/polars-core/src/chunked_array/arithmetic/numeric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,7 @@ where
self,
rhs,
<T::Native as ArrayArithmetics>::div,
// Adding zero to divisor ensures x/0 becomes +infinity, ignoring
// the sign of the zero.
|lhs, rhs| lhs / (rhs + T::Native::zero()),
|lhs, rhs| lhs / rhs,
)
}
}
Expand Down Expand Up @@ -196,9 +194,7 @@ where
self,
rhs,
|a, b| arity_assign::binary(a, b, |a, b| a / b),
// Adding zero to divisor ensures x/0 becomes +infinity, ignoring
// the sign of the zero.
|lhs, rhs| lhs / (rhs + T::Native::zero()),
|lhs, rhs| lhs / rhs,
)
}
}
Expand Down Expand Up @@ -360,10 +356,7 @@ where
type Output = ChunkedArray<T>;

fn div(self, rhs: N) -> Self::Output {
// Adding zero to divisor ensures x/0 becomes +infinity, ignoring
// the sign of the zero.
#[allow(clippy::suspicious_arithmetic_impl)]
(&self).div(rhs + N::zero())
(&self).div(rhs)
}
}

Expand Down
15 changes: 6 additions & 9 deletions docs/user-guide/concepts/data-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,12 @@ To learn more about the internal representation of these data types, check the [

`Polars` generally follows the IEEE 754 floating point standard for `Float32` and `Float64`, with some exceptions:

- `Polars` does not support signed zero and conceptually only has a single zero value.
- `Polars` has a single canonical NaN representation and does not support signed NaNs or NaN payloads.
- The canonical NaN compares equal to itself, and greater than any non-NaN value.
- Any NaN compares equal to any other NaN, and greater than any non-NaN value.
- Operations do not guarantee any particular behavior on the sign of zero or NaN,
nor on the payload of NaN values. This is not just limited to arithmetic operations,
e.g. a sort or group by operation may canonicalize all zeroes to +0 and all NaNs
to a positive NaN without payload for efficient equality checks.

The above canonicalization should be consistently applied for user-facing data, arithmetic
and file-format I/O, but may be violated for zero-copy in-memory export. In such
cases `Polars` simply provides no guarantees around which sign or payload NaNs/zeros
have.

Finally, `Polars` always attempts to provide reasonably accurate results for floating point computations, but does not provide guarantees
`Polars` always attempts to provide reasonably accurate results for floating point computations, but does not provide guarantees
on the error unless mentioned otherwise. Generally speaking 100% accurate results are infeasibly expensive to acquire (requiring
much larger internal representations than 64-bit floats), and thus some error is always to be expected.

0 comments on commit 3b40c7a

Please sign in to comment.