jhorstmann commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487429355
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
I had a bit of a disagreement with a coworker about this clippy warning
before and would like to hear your opinion. IMO in most of those tests
(especially casts, or when the inputs don't have a fractional part) we are
expecting an exact value, not something close to that value. Writing the
assertion like that makes the tests harder to read and also would make me
wonder in which circumstances the kernel would do some rounding.
Some tests like this could also be rewritten such that all input number can
be exactly represented in floating point, following a pattern like x/(2^y), for
example 1.125 or 2.25.
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
I had a bit of a disagreement with a coworker about this clippy warning
before and would like to hear your opinion. IMO in most of those tests
(especially casts, or when the inputs don't have a fractional part) we are
expecting an exact value, not something close to that value. Writing the
assertion like that makes the tests harder to read and also would make me
wonder in which circumstances the kernel would do some rounding.
Some tests like this could also be rewritten such that all input number can
be exactly represented in floating point, following a pattern like x/(2^y), for
example 1.125 or 2.25.
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
I had a bit of a disagreement with a coworker about this clippy warning
before and would like to hear your opinion. IMO in most of those tests
(especially casts, or when the inputs don't have a fractional part) we are
expecting an exact value, not something close to that value. Writing the
assertion like that makes the tests harder to read and also would make me
wonder in which circumstances the kernel would do some rounding.
Some tests like this could also be rewritten such that all input number can
be exactly represented in floating point, following a pattern like x/(2^y), for
example 1.125 or 2.25.
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
I had a bit of a disagreement with a coworker about this clippy warning
before and would like to hear your opinion. IMO in most of those tests
(especially casts, or when the inputs don't have a fractional part) we are
expecting an exact value, not something close to that value. Writing the
assertion like that makes the tests harder to read and also would make me
wonder in which circumstances the kernel would do some rounding.
Some tests like this could also be rewritten such that all input number can
be exactly represented in floating point, following a pattern like x/(2^y), for
example 1.125 or 2.25.
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
I had a bit of a disagreement with a coworker about this clippy warning
before and would like to hear your opinion. IMO in most of those tests
(especially casts, or when the inputs don't have a fractional part) we are
expecting an exact value, not something close to that value. Writing the
assertion like that makes the tests harder to read and also would make me
wonder in which circumstances the kernel would do some rounding.
Some tests like this could also be rewritten such that all input number can
be exactly represented in floating point, following a pattern like x/(2^y), for
example 1.125 or 2.25.
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
I had a bit of a disagreement with a coworker about this clippy warning
before and would like to hear your opinion. IMO in most of those tests
(especially casts, or when the inputs don't have a fractional part) we are
expecting an exact value, not something close to that value. Writing the
assertion like that makes the tests harder to read and also would make me
wonder in which circumstances the kernel would do some rounding.
Some tests like this could also be rewritten such that all input number can
be exactly represented in floating point, following a pattern like x/(2^y), for
example 1.125 or 2.25.
##########
File path: rust/arrow/src/compute/kernels/aggregate.rs
##########
@@ -121,7 +121,7 @@ mod tests {
#[test]
fn test_primitive_array_float_sum() {
let a = Float64Array::from(vec![1.1, 2.2, 3.3, 4.4, 5.5]);
- assert_eq!(16.5, sum(&a).unwrap());
+ assert!(16.5 - sum(&a).unwrap() < f64::EPSILON);
Review comment:
Readability in the sense of understanding the intent of a test. Reading
a test should help understand the specified behaviour of some code. So when I'm
reading a test that asserts with an epsilon while the ieee 754 standard
promises an exact result for those inputs, I'm wondering why the implementation
is allowed to have such imprecision. That applies especially to the cast
kernels, `3 as f64` should be exactly `3.0`, an implementation returning
`2.999999...` should fail that testcase.
Note I'm talking about unit tests of simple operations with small known
inputs and outputs. For more complex (nested, or trigonometric) operations
assertions with some epsilon are totally fine, but then the epsilon also
depends on the scale of the inputs and `f64::EPSILON` is a bit of an arbitrary
choice.
Instead of replacing all floating point assertions with a macro I would
propose to look through those tests and decide whether the result is expected
to be an exact match, and then either use `#[allow(clippy::float_cmp)]` or two
different assert macros for exact/with epsilon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
