From 3bd9c1ab552264d2b02a4214a9ae1dbc8b19ce58 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Mon, 11 Mar 2019 16:53:48 -0400 Subject: [PATCH 1/2] Clarify which (arg)min/max index/value is returned For performance, it's beneficial to avoid guaranteeing a particular iteration order. For example, the current implementation of `min` may return any of the minima because the iteration order of `ArrayBase.fold()` is unspecified. The current implementation of `argmin` does always return the first minimum (in logical order) since `ArrayBase.indexed_iter()` always iterates in logical order, but we may want to optimize the iteration order in the future. --- src/quantile.rs | 28 ++++++++++++++++++++++++++-- tests/quantile.rs | 6 ++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/quantile.rs b/src/quantile.rs index a396edde..76c9c457 100644 --- a/src/quantile.rs +++ b/src/quantile.rs @@ -182,7 +182,7 @@ where S: Data, D: Dimension, { - /// Finds the first index of the minimum value of the array. + /// Finds the index of the minimum value of the array. /// /// Returns `None` if any of the pairwise orderings tested by the function /// are undefined. (For example, this occurs if there are any @@ -190,6 +190,10 @@ where /// /// Returns `None` if the array is empty. /// + /// 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 /// /// ``` @@ -214,12 +218,20 @@ where /// floating-point NaN values in the array.) /// /// Additionally, returns `None` if the array is empty. + /// + /// Even if there are multiple (equal) elements that are minima, only one + /// is returned. (Which one is returned is unspecified and may depend on + /// the memory layout of the array.) fn min(&self) -> Option<&A> where A: PartialOrd; /// Finds the elementwise minimum of the array, skipping NaN values. /// + /// Even if there are multiple (equal) elements that are minima, only one + /// is returned. (Which one is returned is unspecified and may depend on + /// the memory layout of the array.) + /// /// **Warning** This method will return a NaN value if none of the values /// in the array are non-NaN values. Note that the NaN value might not be /// in the array. @@ -228,7 +240,7 @@ where A: MaybeNan, A::NotNan: Ord; - /// Finds the first index of the maximum value of the array. + /// Finds the index of the maximum value of the array. /// /// Returns `None` if any of the pairwise orderings tested by the function /// are undefined. (For example, this occurs if there are any @@ -236,6 +248,10 @@ where /// /// Returns `None` if the array is empty. /// + /// 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 /// /// ``` @@ -260,12 +276,20 @@ where /// floating-point NaN values in the array.) /// /// Additionally, returns `None` if the array is empty. + /// + /// Even if there are multiple (equal) elements that are maxima, only one + /// is returned. (Which one is returned is unspecified and may depend on + /// the memory layout of the array.) fn max(&self) -> Option<&A> where A: PartialOrd; /// Finds the elementwise maximum of the array, skipping NaN values. /// + /// Even if there are multiple (equal) elements that are maxima, only one + /// is returned. (Which one is returned is unspecified and may depend on + /// the memory layout of the array.) + /// /// **Warning** This method will return a NaN value if none of the values /// in the array are non-NaN values. Note that the NaN value might not be /// in the array. diff --git a/tests/quantile.rs b/tests/quantile.rs index 4e799d2f..dc14e601 100644 --- a/tests/quantile.rs +++ b/tests/quantile.rs @@ -20,7 +20,8 @@ fn test_argmin() { assert_eq!(a.argmin(), None); let a = array![[1, 0, 3], [2, 0, 6]]; - assert_eq!(a.argmin(), Some((0, 1))); + let argmin = a.argmin(); + assert!(argmin == Some((0, 1)) || argmin == Some((1, 1))); let a: Array2 = array![[], []]; assert_eq!(a.argmin(), None); @@ -65,7 +66,8 @@ fn test_argmax() { assert_eq!(a.argmax(), None); let a = array![[1, 5, 6], [2, 0, 6]]; - assert_eq!(a.argmax(), Some((0, 2))); + let argmax = a.argmax(); + assert!(argmax == Some((0, 2)) || argmax == Some((1, 2))); let a: Array2 = array![[], []]; assert_eq!(a.argmax(), None); From dbcebde36fb59795a0920c8ca0a3d53327cd9fe4 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Tue, 12 Mar 2019 22:05:21 -0400 Subject: [PATCH 2/2] Replace some tests with argmin/max_matches_min/max See discussion at https://github.com/jturner314/ndarray-stats/pull/32#discussion_r264437350 --- tests/quantile.rs | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/tests/quantile.rs b/tests/quantile.rs index dc14e601..31b51907 100644 --- a/tests/quantile.rs +++ b/tests/quantile.rs @@ -1,12 +1,14 @@ #[macro_use(array)] extern crate ndarray; extern crate ndarray_stats; +extern crate quickcheck; use ndarray::prelude::*; use ndarray_stats::{ interpolate::{Higher, Linear, Lower, Midpoint, Nearest}, Quantile1dExt, QuantileExt, }; +use quickcheck::quickcheck; #[test] fn test_argmin() { @@ -19,14 +21,17 @@ fn test_argmin() { let a = array![[1., 5., 3.], [2., ::std::f64::NAN, 6.]]; assert_eq!(a.argmin(), None); - let a = array![[1, 0, 3], [2, 0, 6]]; - let argmin = a.argmin(); - assert!(argmin == Some((0, 1)) || argmin == Some((1, 1))); - let a: Array2 = array![[], []]; assert_eq!(a.argmin(), None); } +quickcheck! { + fn argmin_matches_min(data: Vec) -> bool { + let a = Array1::from(data); + a.argmin().map(|i| a[i]) == a.min().cloned() + } +} + #[test] fn test_min() { let a = array![[1, 5, 3], [2, 0, 6]]; @@ -65,14 +70,17 @@ fn test_argmax() { let a = array![[1., 5., 3.], [2., ::std::f64::NAN, 6.]]; assert_eq!(a.argmax(), None); - let a = array![[1, 5, 6], [2, 0, 6]]; - let argmax = a.argmax(); - assert!(argmax == Some((0, 2)) || argmax == Some((1, 2))); - let a: Array2 = array![[], []]; assert_eq!(a.argmax(), None); } +quickcheck! { + fn argmax_matches_max(data: Vec) -> bool { + let a = Array1::from(data); + a.argmax().map(|i| a[i]) == a.max().cloned() + } +} + #[test] fn test_max() { let a = array![[1, 5, 7], [2, 0, 6]];