You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/12 15:24:04 UTC
[GitHub] [arrow] nevi-me opened a new pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
nevi-me opened a new pull request #8173:
URL: https://github.com/apache/arrow/pull/8173
This is a followup of https://issues.apache.org/jira/browse/ARROW-9979
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#issuecomment-691506541
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] sunchao commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487436938
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#issuecomment-691506541
https://issues.apache.org/jira/browse/ARROW-9980
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487438276
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#issuecomment-691506541
https://issues.apache.org/jira/browse/ARROW-9980
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487447527
##########
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 like the approach of a macro, we can add it in a follow-up
##########
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 like the approach of a macro, we can add it in a follow-up
##########
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 like the approach of a macro, we can add it in a follow-up
##########
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 like the approach of a macro, we can add it in a follow-up
##########
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 can do this in the coming 2 weeks. I would agree with the approach of turning off the lint where it matters, which I think would be almost all cases.
I had to merge this PR to bank the time (spent much of the Saturday working on this instead of a lot of other work)
##########
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 can do this in the coming 2 weeks. I would agree with the approach of turning off the lint where it matters, which I think would be almost all cases.
I had to merge this PR to bank the time (spent much of the Saturday working on this instead of a lot of other work) and because I already have 2 other PRs that I decided to prepare on top of the clippy lints
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] andygrove commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
andygrove commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487438276
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
##########
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:
The results could vary depending on the hardware we are running on. For now, we are assuming that everything is running on CPU but there is no reason why we couldn't have GPU versions of these kernels in the future, and there could be differences in precision in this case.
https://docs.nvidia.com/cuda/floating-point/index.html
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] sunchao commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487436938
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#issuecomment-691506541
https://issues.apache.org/jira/browse/ARROW-9980
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nevi-me closed pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #8173:
URL: https://github.com/apache/arrow/pull/8173
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] jhorstmann commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [arrow] sunchao commented on a change in pull request #8173: ARROW-9980: [Rust] [Parquet] Fix clippy lints
Posted by GitBox <gi...@apache.org>.
sunchao commented on a change in pull request #8173:
URL: https://github.com/apache/arrow/pull/8173#discussion_r487436938
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
##########
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:
If it is about readability, perhaps we can define a macro like `assert_float_eq` and wrap this logic inside?
----------------------------------------------------------------
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:
users@infra.apache.org