From 2250a659ca7e5c13a102526242155cde373e014d Mon Sep 17 00:00:00 2001 From: Son Date: Tue, 12 Mar 2019 10:10:23 +1100 Subject: [PATCH 01/11] Implement argmin_skipnan --- src/quantile.rs | 45 +++++++++++++++++++++++++++++++++++++++++++++ tests/quantile.rs | 21 +++++++++++++++++++++ 2 files changed, 66 insertions(+) diff --git a/src/quantile.rs b/src/quantile.rs index 76c9c457..aefcce51 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -211,6 +211,31 @@ where where A: PartialOrd; + /// Finds the first index of the minimum value of the array ignoring nan values. + /// + /// Returns `None` if the array is empty. + /// + /// **Warning** This method will return a None value if none of the values + /// in the array are non-NaN values. + /// + /// # Example + /// + /// ``` + /// extern crate ndarray; + /// extern crate ndarray_stats; + /// + /// use ndarray::array; + /// use ndarray_stats::QuantileExt; + /// + /// let a = array![[::std::f64::NAN, 3., 5.], + /// [2., 0., 6.]]; + /// assert_eq!(a.argmin_skipnan(), Some((1, 1))); + /// ``` + fn argmin_skipnan(&self) -> Option + where + A: MaybeNan, + A::NotNan: Ord + std::fmt::Debug; + /// Finds the elementwise minimum of the array. /// /// Returns `None` if any of the pairwise orderings tested by the function @@ -369,6 +394,26 @@ where Some(current_pattern_min) } + fn argmin_skipnan(&self) -> Option + where + A: MaybeNan, + A::NotNan: Ord + std::fmt::Debug, + { + let mut current_min = self.first().and_then(|v| v.try_as_not_nan()); + let mut current_pattern_min = D::zeros(self.ndim()).into_pattern(); + for (pattern, elem) in self.indexed_iter() { + let elem_not_nan = elem.try_as_not_nan(); + if elem_not_nan.is_some() + && (current_min.is_none() + || elem_not_nan.partial_cmp(¤t_min) == Some(cmp::Ordering::Less)) + { + current_pattern_min = pattern; + current_min = elem_not_nan; + } + } + current_min.map({ |_| current_pattern_min }) + } + fn min(&self) -> Option<&A> where A: PartialOrd, diff --git a/tests/quantile.rs b/tests/quantile.rs index 31b51907..b1e63c4b 100644 --- a/tests/quantile.rs +++ b/tests/quantile.rs @@ -32,6 +32,27 @@ quickcheck! { } } +#[test] +fn test_argmin_skipnan() { + let a = array![[1., 5., 3.], [2., 0., 6.]]; + assert_eq!(a.argmin_skipnan(), Some((1, 1))); + + let a = array![[1., 5., 3.], [2., ::std::f64::NAN, 6.]]; + assert_eq!(a.argmin_skipnan(), Some((0, 0))); + + let a = array![[::std::f64::NAN, 5., 3.], [2., ::std::f64::NAN, 6.]]; + assert_eq!(a.argmin_skipnan(), Some((1, 0))); + + let a: Array2 = array![[], []]; + assert_eq!(a.argmin_skipnan(), None); + + let a = array![ + [::std::f64::NAN, ::std::f64::NAN], + [::std::f64::NAN, ::std::f64::NAN] + ]; + assert_eq!(a.argmin_skipnan(), None); +} + #[test] fn test_min() { let a = array![[1, 5, 3], [2, 0, 6]]; From 7aa5e83e7b376b60ed84ee7f0981d1b8e1a2f55e Mon Sep 17 00:00:00 2001 From: Son Date: Tue, 12 Mar 2019 10:18:03 +1100 Subject: [PATCH 02/11] Implement argmax_skipnan --- src/quantile.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++--- tests/quantile.rs | 26 ++++++++++++++++++++---- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index aefcce51..e71af7c3 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -211,7 +211,7 @@ where where A: PartialOrd; - /// Finds the first index of the minimum value of the array ignoring nan values. + /// Finds the first index of the minimum value of the array skipping NaN values. /// /// Returns `None` if the array is empty. /// @@ -234,7 +234,7 @@ where fn argmin_skipnan(&self) -> Option where A: MaybeNan, - A::NotNan: Ord + std::fmt::Debug; + A::NotNan: Ord; /// Finds the elementwise minimum of the array. /// @@ -294,6 +294,31 @@ where where A: PartialOrd; + /// Finds the first index of the maximum value of the array skipping NaN values. + /// + /// Returns `None` if the array is empty. + /// + /// **Warning** This method will return a None value if none of the values + /// in the array are non-NaN values. + /// + /// # Example + /// + /// ``` + /// extern crate ndarray; + /// extern crate ndarray_stats; + /// + /// use ndarray::array; + /// use ndarray_stats::QuantileExt; + /// + /// let a = array![[::std::f64::NAN, 3., 5.], + /// [2., 0., 6.]]; + /// assert_eq!(a.argmax_skipnan(), Some((1, 2))); + /// ``` + fn argmax_skipnan(&self) -> Option + where + A: MaybeNan, + A::NotNan: Ord; + /// Finds the elementwise maximum of the array. /// /// Returns `None` if any of the pairwise orderings tested by the function @@ -397,7 +422,7 @@ where fn argmin_skipnan(&self) -> Option where A: MaybeNan, - A::NotNan: Ord + std::fmt::Debug, + A::NotNan: Ord, { let mut current_min = self.first().and_then(|v| v.try_as_not_nan()); let mut current_pattern_min = D::zeros(self.ndim()).into_pattern(); @@ -456,6 +481,26 @@ where Some(current_pattern_max) } + fn argmax_skipnan(&self) -> Option + where + A: MaybeNan, + A::NotNan: Ord, + { + let mut current_max = self.first().and_then(|v| v.try_as_not_nan()); + let mut current_pattern_max = D::zeros(self.ndim()).into_pattern(); + for (pattern, elem) in self.indexed_iter() { + let elem_not_nan = elem.try_as_not_nan(); + if elem_not_nan.is_some() + && (current_max.is_none() + || elem_not_nan.partial_cmp(¤t_max) == Some(cmp::Ordering::Greater)) + { + current_pattern_max = pattern; + current_max = elem_not_nan; + } + } + current_max.map({ |_| current_pattern_max }) + } + fn max(&self) -> Option<&A> where A: PartialOrd, diff --git a/tests/quantile.rs b/tests/quantile.rs index b1e63c4b..a3a0b1cb 100644 --- a/tests/quantile.rs +++ b/tests/quantile.rs @@ -46,10 +46,7 @@ fn test_argmin_skipnan() { let a: Array2 = array![[], []]; assert_eq!(a.argmin_skipnan(), None); - let a = array![ - [::std::f64::NAN, ::std::f64::NAN], - [::std::f64::NAN, ::std::f64::NAN] - ]; + let a = arr2(&[[::std::f64::NAN; 2]; 2]); assert_eq!(a.argmin_skipnan(), None); } @@ -102,6 +99,27 @@ quickcheck! { } } +#[test] +fn test_argmax_skipnan() { + let a = array![[1., 5., 3.], [2., 0., 6.]]; + assert_eq!(a.argmax_skipnan(), Some((1, 2))); + + let a = array![[1., 5., 3.], [2., ::std::f64::NAN, ::std::f64::NAN]]; + assert_eq!(a.argmax_skipnan(), Some((0, 1))); + + let a = array![ + [::std::f64::NAN, ::std::f64::NAN, 3.], + [2., ::std::f64::NAN, 6.] + ]; + assert_eq!(a.argmax_skipnan(), Some((1, 2))); + + let a: Array2 = array![[], []]; + assert_eq!(a.argmax_skipnan(), None); + + let a = arr2(&[[::std::f64::NAN; 2]; 2]); + assert_eq!(a.argmax_skipnan(), None); +} + #[test] fn test_max() { let a = array![[1, 5, 7], [2, 0, 6]]; From 4a7c1788064856f2f80d8120f346a54de3671b17 Mon Sep 17 00:00:00 2001 From: Son Date: Wed, 13 Mar 2019 09:24:35 +1100 Subject: [PATCH 03/11] Loosen the rule for argmin max related methods --- src/quantile.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index e71af7c3..733aabed 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -211,12 +211,14 @@ where where A: PartialOrd; - /// Finds the first index of the minimum value of the array skipping NaN values. + /// Finds the index of the minimum value of the array skipping NaN values. /// - /// Returns `None` if the array is empty. + /// Returns `None` if the array is empty or none of the values in the array + /// are non-NaN values. /// - /// **Warning** This method will return a None value if none of the values - /// in the array are non-NaN values. + /// Even if there are multiple (equal) elements that are minima, only one + /// index is returned. (Which one is returned is unspecified and may depend + /// on the memory layout of the array.) /// /// # Example /// @@ -294,12 +296,14 @@ where where A: PartialOrd; - /// Finds the first index of the maximum value of the array skipping NaN values. + /// Finds the index of the maximum value of the array skipping NaN values. /// - /// Returns `None` if the array is empty. + /// Returns `None` if the array is empty or none of the values in the array + /// are non-NaN values. /// - /// **Warning** This method will return a None value if none of the values - /// in the array are non-NaN values. + /// Even if there are multiple (equal) elements that are maxima, only one + /// index is returned. (Which one is returned is unspecified and may depend + /// on the memory layout of the array.) /// /// # Example /// From 5f63c86826f32b2ebc3bbed788e4f2cce252ad29 Mon Sep 17 00:00:00 2001 From: Son Date: Sat, 16 Mar 2019 10:27:26 +1100 Subject: [PATCH 04/11] Make returning code clearer --- src/quantile.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index 733aabed..afe14560 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -440,7 +440,12 @@ where current_min = elem_not_nan; } } - current_min.map({ |_| current_pattern_min }) + + if current_min == None { + None + } else { + Some(current_pattern_min) + } } fn min(&self) -> Option<&A> @@ -502,7 +507,12 @@ where current_max = elem_not_nan; } } - current_max.map({ |_| current_pattern_max }) + + if current_max == None { + None + } else { + Some(current_pattern_max) + } } fn max(&self) -> Option<&A> From f33f7321ad56246be82d14fb16dc59ed948c6bbd Mon Sep 17 00:00:00 2001 From: Son Date: Sat, 16 Mar 2019 10:38:02 +1100 Subject: [PATCH 05/11] Add quickcheck for argmin_skipnan, argmax_skipnan --- tests/quantile.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/quantile.rs b/tests/quantile.rs index a3a0b1cb..c4c9e6f2 100644 --- a/tests/quantile.rs +++ b/tests/quantile.rs @@ -50,6 +50,13 @@ fn test_argmin_skipnan() { assert_eq!(a.argmin_skipnan(), None); } +quickcheck! { + fn argmin_skipnan_matches_min(data: Vec) -> bool { + let a = Array1::from(data); + a.argmin_skipnan().map(|i| a[i]) == a.min().cloned() + } +} + #[test] fn test_min() { let a = array![[1, 5, 3], [2, 0, 6]]; @@ -120,6 +127,13 @@ fn test_argmax_skipnan() { assert_eq!(a.argmax_skipnan(), None); } +quickcheck! { + fn argmax_skipnan_matches_max(data: Vec) -> bool { + let a = Array1::from(data); + a.argmax_skipnan().map(|i| a[i]) == a.max().cloned() + } +} + #[test] fn test_max() { let a = array![[1, 5, 7], [2, 0, 6]]; From e602636b385d53f8c6962c95c7f5bcf17416cf00 Mon Sep 17 00:00:00 2001 From: Son Date: Sat, 16 Mar 2019 12:04:29 +1100 Subject: [PATCH 06/11] Use `fold` instead of `for` --- src/quantile.rs | 68 +++++++++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index afe14560..3c78fc7d 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -428,23 +428,29 @@ where A: MaybeNan, A::NotNan: Ord, { - let mut current_min = self.first().and_then(|v| v.try_as_not_nan()); - let mut current_pattern_min = D::zeros(self.ndim()).into_pattern(); - for (pattern, elem) in self.indexed_iter() { - let elem_not_nan = elem.try_as_not_nan(); - if elem_not_nan.is_some() - && (current_min.is_none() - || elem_not_nan.partial_cmp(¤t_min) == Some(cmp::Ordering::Less)) - { - current_pattern_min = pattern; - current_min = elem_not_nan; - } - } + let first = self.first().and_then(|v| v.try_as_not_nan()); + let mut pattern_min = D::zeros(self.ndim()).into_pattern(); + + let min = self + .indexed_iter() + .fold(first, |current_min, (pattern, elem)| { + let elem_not_nan = elem.try_as_not_nan(); + + if elem_not_nan.is_some() + && (current_min.is_none() + || elem_not_nan.cmp(¤t_min) == cmp::Ordering::Less) + { + pattern_min = pattern; + elem_not_nan + } else { + current_min + } + }); - if current_min == None { + if min == None { None } else { - Some(current_pattern_min) + Some(pattern_min) } } @@ -495,23 +501,29 @@ where A: MaybeNan, A::NotNan: Ord, { - let mut current_max = self.first().and_then(|v| v.try_as_not_nan()); - let mut current_pattern_max = D::zeros(self.ndim()).into_pattern(); - for (pattern, elem) in self.indexed_iter() { - let elem_not_nan = elem.try_as_not_nan(); - if elem_not_nan.is_some() - && (current_max.is_none() - || elem_not_nan.partial_cmp(¤t_max) == Some(cmp::Ordering::Greater)) - { - current_pattern_max = pattern; - current_max = elem_not_nan; - } - } + let first = self.first().and_then(|v| v.try_as_not_nan()); + let mut pattern_max = D::zeros(self.ndim()).into_pattern(); + + let max = self + .indexed_iter() + .fold(first, |current_max, (pattern, elem)| { + let elem_not_nan = elem.try_as_not_nan(); + + if elem_not_nan.is_some() + && (current_max.is_none() + || elem_not_nan.cmp(¤t_max) == cmp::Ordering::Greater) + { + pattern_max = pattern; + elem_not_nan + } else { + current_max + } + }); - if current_max == None { + if max == None { None } else { - Some(current_pattern_max) + Some(pattern_max) } } From 76d7201544995c0bafc797048ea0813638ba77a3 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 17 Mar 2019 02:47:01 -0400 Subject: [PATCH 07/11] Add indexed_fold_skipnan to MaybeNanExt --- src/maybe_nan/mod.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/maybe_nan/mod.rs b/src/maybe_nan/mod.rs index 3905fa9b..15adbafd 100644 --- a/src/maybe_nan/mod.rs +++ b/src/maybe_nan/mod.rs @@ -241,6 +241,15 @@ where A: 'a, F: FnMut(B, &'a A::NotNan) -> B; + /// Traverse the non-NaN elements and their indices and apply a fold, + /// returning the resulting value. + /// + /// Elements are visited in arbitrary order. + fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, f: F) -> B + where + A: 'a, + F: FnMut(B, (D::Pattern, &'a A::NotNan)) -> B; + /// Visit each non-NaN element in the array by calling `f` on each element. /// /// Elements are visited in arbitrary order. @@ -302,6 +311,20 @@ where }) } + fn indexed_fold_skipnan<'a, F, B>(&'a self, init: B, mut f: F) -> B + where + A: 'a, + F: FnMut(B, (D::Pattern, &'a A::NotNan)) -> B, + { + self.indexed_iter().fold(init, |acc, (idx, elem)| { + if let Some(not_nan) = elem.try_as_not_nan() { + f(acc, (idx, not_nan)) + } else { + acc + } + }) + } + fn visit_skipnan<'a, F>(&'a self, mut f: F) where A: 'a, From 6917d2f2c66abe52d0feb2b1067bfb0da079dcff Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 17 Mar 2019 02:47:57 -0400 Subject: [PATCH 08/11] Impl argmin/max_skipnan using indexed_fold_skipnan --- src/quantile.rs | 58 ++++++++++++++----------------------------------- 1 file changed, 16 insertions(+), 42 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index 3c78fc7d..dd6447ab 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -428,30 +428,17 @@ where A: MaybeNan, A::NotNan: Ord, { - let first = self.first().and_then(|v| v.try_as_not_nan()); let mut pattern_min = D::zeros(self.ndim()).into_pattern(); - - let min = self - .indexed_iter() - .fold(first, |current_min, (pattern, elem)| { - let elem_not_nan = elem.try_as_not_nan(); - - if elem_not_nan.is_some() - && (current_min.is_none() - || elem_not_nan.cmp(¤t_min) == cmp::Ordering::Less) - { + let min = self.indexed_fold_skipnan(None, |current_min, (pattern, elem)| { + Some(match current_min { + Some(m) if m <= elem => m, + _ => { pattern_min = pattern; - elem_not_nan - } else { - current_min + elem } - }); - - if min == None { - None - } else { - Some(pattern_min) - } + }) + }); + min.map(|_| pattern_min) } fn min(&self) -> Option<&A> @@ -501,30 +488,17 @@ where A: MaybeNan, A::NotNan: Ord, { - let first = self.first().and_then(|v| v.try_as_not_nan()); let mut pattern_max = D::zeros(self.ndim()).into_pattern(); - - let max = self - .indexed_iter() - .fold(first, |current_max, (pattern, elem)| { - let elem_not_nan = elem.try_as_not_nan(); - - if elem_not_nan.is_some() - && (current_max.is_none() - || elem_not_nan.cmp(¤t_max) == cmp::Ordering::Greater) - { + let max = self.indexed_fold_skipnan(None, |current_max, (pattern, elem)| { + Some(match current_max { + Some(m) if m >= elem => m, + _ => { pattern_max = pattern; - elem_not_nan - } else { - current_max + elem } - }); - - if max == None { - None - } else { - Some(pattern_max) - } + }) + }); + max.map(|_| pattern_max) } fn max(&self) -> Option<&A> From fd1104cd8e88203d543fea29546be3bafcc333cd Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 17 Mar 2019 02:55:21 -0400 Subject: [PATCH 09/11] Fix argmin/max_skipnan quickcheck tests The old tests were incorrect because `min`/`max` return `None` when there are *any* NaN values (or the array is empty), while `argmin/max_skipnan` should return `None` only when *all* the values are NaNs (or the array is empty). This wasn't caught earlier because the `quickcheck::Arbitrary` implementation for `f32` generates only finite values. To make sure the behavior with NaN values is properly tested, the element type in the test has been changed to `Option`. --- tests/quantile.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/quantile.rs b/tests/quantile.rs index c4c9e6f2..3e5ba53b 100644 --- a/tests/quantile.rs +++ b/tests/quantile.rs @@ -51,9 +51,15 @@ fn test_argmin_skipnan() { } quickcheck! { - fn argmin_skipnan_matches_min(data: Vec) -> bool { + fn argmin_skipnan_matches_min_skipnan(data: Vec>) -> bool { let a = Array1::from(data); - a.argmin_skipnan().map(|i| a[i]) == a.min().cloned() + let min = a.min_skipnan(); + let argmin = a.argmin_skipnan(); + if min.is_none() { + argmin == None + } else { + a[argmin.unwrap()] == *min + } } } @@ -128,9 +134,15 @@ fn test_argmax_skipnan() { } quickcheck! { - fn argmax_skipnan_matches_max(data: Vec) -> bool { + fn argmax_skipnan_matches_max_skipnan(data: Vec>) -> bool { let a = Array1::from(data); - a.argmax_skipnan().map(|i| a[i]) == a.max().cloned() + let max = a.max_skipnan(); + let argmax = a.argmax_skipnan(); + if max.is_none() { + argmax == None + } else { + a[argmax.unwrap()] == *max + } } } From 05b824bbca1627e700a993d2e317a2729a0bc414 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 17 Mar 2019 03:44:17 -0400 Subject: [PATCH 10/11] Replace min/max.map with if for clarity --- src/quantile.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index dd6447ab..470ba15b 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -438,7 +438,11 @@ where } }) }); - min.map(|_| pattern_min) + if min.is_some() { + Some(pattern_min) + } else { + None + } } fn min(&self) -> Option<&A> @@ -498,7 +502,11 @@ where } }) }); - max.map(|_| pattern_max) + if max.is_some() { + Some(pattern_max) + } else { + None + } } fn max(&self) -> Option<&A> From c829d5df99ed0433dec8878c036eaa22018d9ea7 Mon Sep 17 00:00:00 2001 From: Son Date: Fri, 22 Mar 2019 21:10:20 +1100 Subject: [PATCH 11/11] Add () to make the match clearer --- src/quantile.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/quantile.rs b/src/quantile.rs index 470ba15b..1b4b4fd6 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -431,7 +431,7 @@ where let mut pattern_min = D::zeros(self.ndim()).into_pattern(); let min = self.indexed_fold_skipnan(None, |current_min, (pattern, elem)| { Some(match current_min { - Some(m) if m <= elem => m, + Some(m) if (m <= elem) => m, _ => { pattern_min = pattern; elem