From e6a0e9349b9364e1c870473320968d9fdc8b5115 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 5 May 2019 16:43:42 -0400 Subject: [PATCH 1/3] Make geomspace return None instead of panicking This helps to make the user more aware of the constraints on `geomspace` and allows recovering in case of bad input. --- src/geomspace.rs | 43 +++++++++++++++++----------------------- src/impl_constructors.rs | 18 +++++++++++------ 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/geomspace.rs b/src/geomspace.rs index 63136190f..9279f6c5b 100644 --- a/src/geomspace.rs +++ b/src/geomspace.rs @@ -71,21 +71,17 @@ impl ExactSizeIterator for Geomspace where Geomspace: Iterator {} /// /// Iterator element type is `F`, where `F` must be either `f32` or `f64`. /// -/// **Panics** if the interval `[a, b]` contains zero (including the end points). +/// Returns `None` if `start` and `end` have different signs or if either one +/// is zero. Conceptually, this means that in order to obtain a `Some` result, +/// `end / start` must be positive. #[inline] -pub fn geomspace(a: F, b: F, n: usize) -> Geomspace +pub fn geomspace(a: F, b: F, n: usize) -> Option> where F: Float, { - assert!( - a != F::zero() && b != F::zero(), - "Start and/or end of geomspace cannot be zero.", - ); - assert!( - a.is_sign_negative() == b.is_sign_negative(), - "Logarithmic interval cannot cross 0." - ); - + if a == F::zero() || b == F::zero() || a.is_sign_negative() != b.is_sign_negative() { + return None; + } let log_a = a.abs().ln(); let log_b = b.abs().ln(); let step = if n > 1 { @@ -94,13 +90,13 @@ where } else { F::zero() }; - Geomspace { + Some(Geomspace { sign: a.signum(), start: log_a, step: step, index: 0, len: n, - } + }) } #[cfg(test)] @@ -113,22 +109,22 @@ mod tests { use approx::assert_abs_diff_eq; use crate::{arr1, Array1}; - let array: Array1<_> = geomspace(1e0, 1e3, 4).collect(); + let array: Array1<_> = geomspace(1e0, 1e3, 4).unwrap().collect(); assert_abs_diff_eq!(array, arr1(&[1e0, 1e1, 1e2, 1e3]), epsilon = 1e-12); - let array: Array1<_> = geomspace(1e3, 1e0, 4).collect(); + let array: Array1<_> = geomspace(1e3, 1e0, 4).unwrap().collect(); assert_abs_diff_eq!(array, arr1(&[1e3, 1e2, 1e1, 1e0]), epsilon = 1e-12); - let array: Array1<_> = geomspace(-1e3, -1e0, 4).collect(); + let array: Array1<_> = geomspace(-1e3, -1e0, 4).unwrap().collect(); assert_abs_diff_eq!(array, arr1(&[-1e3, -1e2, -1e1, -1e0]), epsilon = 1e-12); - let array: Array1<_> = geomspace(-1e0, -1e3, 4).collect(); + let array: Array1<_> = geomspace(-1e0, -1e3, 4).unwrap().collect(); assert_abs_diff_eq!(array, arr1(&[-1e0, -1e1, -1e2, -1e3]), epsilon = 1e-12); } #[test] fn iter_forward() { - let mut iter = geomspace(1.0f64, 1e3, 4); + let mut iter = geomspace(1.0f64, 1e3, 4).unwrap(); assert!(iter.size_hint() == (4, Some(4))); @@ -143,7 +139,7 @@ mod tests { #[test] fn iter_backward() { - let mut iter = geomspace(1.0f64, 1e3, 4); + let mut iter = geomspace(1.0f64, 1e3, 4).unwrap(); assert!(iter.size_hint() == (4, Some(4))); @@ -157,20 +153,17 @@ mod tests { } #[test] - #[should_panic] fn zero_lower() { - geomspace(0.0, 1.0, 4); + assert!(geomspace(0.0, 1.0, 4).is_none()); } #[test] - #[should_panic] fn zero_upper() { - geomspace(1.0, 0.0, 4); + assert!(geomspace(1.0, 0.0, 4).is_none()); } #[test] - #[should_panic] fn zero_included() { - geomspace(-1.0, 1.0, 4); + assert!(geomspace(-1.0, 1.0, 4).is_none()); } } diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 72dbfd310..03458ed00 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -133,8 +133,9 @@ impl ArrayBase /// end]` with `n` elements geometrically spaced. `A` must be a floating /// point type. /// - /// The interval can be either all positive or all negative; however, it - /// cannot contain 0 (including the end points). + /// Returns `None` if `start` and `end` have different signs or if either + /// one is zero. Conceptually, this means that in order to obtain a `Some` + /// result, `end / start` must be positive. /// /// **Panics** if `n` is greater than `isize::MAX`. /// @@ -142,19 +143,24 @@ impl ArrayBase /// use approx::assert_abs_diff_eq; /// use ndarray::{Array, arr1}; /// + /// # fn example() -> Option<()> { /// # #[cfg(feature = "approx")] { - /// let array = Array::geomspace(1e0, 1e3, 4); + /// let array = Array::geomspace(1e0, 1e3, 4)?; /// assert_abs_diff_eq!(array, arr1(&[1e0, 1e1, 1e2, 1e3]), epsilon = 1e-12); /// - /// let array = Array::geomspace(-1e3, -1e0, 4); + /// let array = Array::geomspace(-1e3, -1e0, 4)?; /// assert_abs_diff_eq!(array, arr1(&[-1e3, -1e2, -1e1, -1e0]), epsilon = 1e-12); /// # } + /// # Some(()) + /// # } + /// # + /// # fn main() { example().unwrap() } /// ``` - pub fn geomspace(start: A, end: A, n: usize) -> Self + pub fn geomspace(start: A, end: A, n: usize) -> Option where A: Float, { - Self::from_vec(to_vec(geomspace::geomspace(start, end, n))) + Some(Self::from_vec(to_vec(geomspace::geomspace(start, end, n)?))) } } From 95ce9e7eb93d9654a184b7ccca6e42cdb09cdf98 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 5 May 2019 16:45:35 -0400 Subject: [PATCH 2/3] Convert (n-1) to F instead subtracting F::one() This makes the implementation clearer and avoids the issue that `One::one()` is defined to be the multiplicative identity, not necessarily `F::from(1)`. --- src/geomspace.rs | 4 ++-- src/linspace.rs | 4 ++-- src/logspace.rs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/geomspace.rs b/src/geomspace.rs index 9279f6c5b..c3b5a4095 100644 --- a/src/geomspace.rs +++ b/src/geomspace.rs @@ -85,8 +85,8 @@ where let log_a = a.abs().ln(); let log_b = b.abs().ln(); let step = if n > 1 { - let nf: F = F::from(n).unwrap(); - (log_b - log_a) / (nf - F::one()) + let num_steps = F::from(n - 1).expect("Converting number of steps to `A` must not fail."); + (log_b - log_a) / num_steps } else { F::zero() }; diff --git a/src/linspace.rs b/src/linspace.rs index 51ede80a7..2654d406d 100644 --- a/src/linspace.rs +++ b/src/linspace.rs @@ -73,8 +73,8 @@ pub fn linspace(a: F, b: F, n: usize) -> Linspace where F: Float { let step = if n > 1 { - let nf: F = F::from(n).unwrap(); - (b - a) / (nf - F::one()) + let num_steps = F::from(n - 1).expect("Converting number of steps to `A` must not fail."); + (b - a) / num_steps } else { F::zero() }; diff --git a/src/logspace.rs b/src/logspace.rs index 7a32f3795..a86ea0545 100644 --- a/src/logspace.rs +++ b/src/logspace.rs @@ -78,8 +78,8 @@ where F: Float, { let step = if n > 1 { - let nf: F = F::from(n).unwrap(); - (b - a) / (nf - F::one()) + let num_steps = F::from(n - 1).expect("Converting number of steps to `A` must not fail."); + (b - a) / num_steps } else { F::zero() }; From 0295d46adab328067b8b689122cce98bfb46ffc8 Mon Sep 17 00:00:00 2001 From: Jim Turner Date: Sun, 5 May 2019 16:50:59 -0400 Subject: [PATCH 3/3] Improve docs of linspace, range, logspace, geomspace This includes a few corrections and clarifications: * The old docs used interval notation in some cases, which was confusing in the case that `start > end`. * `linspace` now has a note describing the difference in behavior from `std::ops::RangeInclusive` for the case that `start > end`. * Panics in conversions from `usize` to `A` are now mentioned. * The docs now say that the element type must implement `Float` instead of saying that it must be `f32` or `f64`. --- src/geomspace.rs | 9 ++++++--- src/impl_constructors.rs | 34 ++++++++++++++++++++-------------- src/linspace.rs | 29 ++++++++++++++++++----------- src/logspace.rs | 7 +++++-- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/geomspace.rs b/src/geomspace.rs index c3b5a4095..285e761a3 100644 --- a/src/geomspace.rs +++ b/src/geomspace.rs @@ -66,14 +66,17 @@ impl ExactSizeIterator for Geomspace where Geomspace: Iterator {} /// An iterator of a sequence of geometrically spaced values. /// -/// The `Geomspace` has `n` elements, where the first element is `a` and the -/// last element is `b`. +/// The `Geomspace` has `n` geometrically spaced elements from `start` to `end` +/// (inclusive). /// -/// Iterator element type is `F`, where `F` must be either `f32` or `f64`. +/// The iterator element type is `F`, where `F` must implement `Float`, e.g. +/// `f32` or `f64`. /// /// Returns `None` if `start` and `end` have different signs or if either one /// is zero. Conceptually, this means that in order to obtain a `Some` result, /// `end / start` must be positive. +/// +/// **Panics** if converting `n - 1` to type `F` fails. #[inline] pub fn geomspace(a: F, b: F, n: usize) -> Option> where diff --git a/src/impl_constructors.rs b/src/impl_constructors.rs index 03458ed00..300ae91c1 100644 --- a/src/impl_constructors.rs +++ b/src/impl_constructors.rs @@ -67,10 +67,16 @@ impl ArrayBase Self::from_vec(iterable.into_iter().collect()) } - /// Create a one-dimensional array from the inclusive interval - /// `[start, end]` with `n` elements. `A` must be a floating point type. + /// Create a one-dimensional array with `n` evenly spaced elements from + /// `start` to `end` (inclusive). `A` must be a floating point type. /// - /// **Panics** if `n` is greater than `isize::MAX`. + /// Note that if `start > end`, the first element will still be `start`, + /// and the following elements will be decreasing. This is different from + /// the behavior of `std::ops::RangeInclusive`, which interprets `start > + /// end` to mean that the range is empty. + /// + /// **Panics** if `n` is greater than `isize::MAX` or if converting `n - 1` + /// to type `A` fails. /// /// ```rust /// use ndarray::{Array, arr1}; @@ -84,9 +90,8 @@ impl ArrayBase Self::from_vec(to_vec(linspace::linspace(start, end, n))) } - /// Create a one-dimensional array from the half-open interval - /// `[start, end)` with elements spaced by `step`. `A` must be a floating - /// point type. + /// Create a one-dimensional array with elements from `start` to `end` + /// (exclusive), incrementing by `step`. `A` must be a floating point type. /// /// **Panics** if the length is greater than `isize::MAX`. /// @@ -102,13 +107,14 @@ impl ArrayBase Self::from_vec(to_vec(linspace::range(start, end, step))) } - /// Create a one-dimensional array with `n` elements logarithmically spaced, - /// with the starting value being `base.powf(start)` and the final one being - /// `base.powf(end)`. `A` must be a floating point type. + /// Create a one-dimensional array with `n` logarithmically spaced + /// elements, with the starting value being `base.powf(start)` and the + /// final one being `base.powf(end)`. `A` must be a floating point type. /// /// If `base` is negative, all values will be negative. /// - /// **Panics** if the length is greater than `isize::MAX`. + /// **Panics** if `n` is greater than `isize::MAX` or if converting `n - 1` + /// to type `A` fails. /// /// ```rust /// use approx::assert_abs_diff_eq; @@ -129,15 +135,15 @@ impl ArrayBase Self::from_vec(to_vec(logspace::logspace(base, start, end, n))) } - /// Create a one-dimensional array from the inclusive interval `[start, - /// end]` with `n` elements geometrically spaced. `A` must be a floating - /// point type. + /// Create a one-dimensional array with `n` geometrically spaced elements + /// from `start` to `end` (inclusive). `A` must be a floating point type. /// /// Returns `None` if `start` and `end` have different signs or if either /// one is zero. Conceptually, this means that in order to obtain a `Some` /// result, `end / start` must be positive. /// - /// **Panics** if `n` is greater than `isize::MAX`. + /// **Panics** if `n` is greater than `isize::MAX` or if converting `n - 1` + /// to type `A` fails. /// /// ```rust /// use approx::assert_abs_diff_eq; diff --git a/src/linspace.rs b/src/linspace.rs index 2654d406d..f70303dca 100644 --- a/src/linspace.rs +++ b/src/linspace.rs @@ -63,11 +63,12 @@ impl ExactSizeIterator for Linspace /// Return an iterator of evenly spaced floats. /// -/// The `Linspace` has `n` elements, where the first -/// element is `a` and the last element is `b`. +/// The `Linspace` has `n` elements from `a` to `b` (inclusive). /// -/// Iterator element type is `F`, where `F` must be -/// either `f32` or `f64`. +/// The iterator element type is `F`, where `F` must implement `Float`, e.g. +/// `f32` or `f64`. +/// +/// **Panics** if converting `n - 1` to type `F` fails. #[inline] pub fn linspace(a: F, b: F, n: usize) -> Linspace where F: Float @@ -86,13 +87,15 @@ pub fn linspace(a: F, b: F, n: usize) -> Linspace } } -/// Return an iterator of floats spaced by `step`, from -/// the half-open interval [a, b). -/// Numerical reasons can result in `b` being included -/// in the result. +/// Return an iterator of floats from `start` to `end` (exclusive), +/// incrementing by `step`. +/// +/// Numerical reasons can result in `b` being included in the result. +/// +/// The iterator element type is `F`, where `F` must implement `Float`, e.g. +/// `f32` or `f64`. /// -/// Iterator element type is `F`, where `F` must be -/// either `f32` or `f64`. +/// **Panics** if converting `((b - a) / step).ceil()` to type `F` fails. #[inline] pub fn range(a: F, b: F, step: F) -> Linspace where F: Float @@ -102,7 +105,11 @@ pub fn range(a: F, b: F, step: F) -> Linspace Linspace { start: a, step: step, - len: steps.to_usize().unwrap(), + len: steps.to_usize().expect( + "Converting the length to `usize` must not fail. The most likely \ + cause of this failure is if the sign of `end - start` is \ + different from the sign of `step`.", + ), index: 0, } } diff --git a/src/logspace.rs b/src/logspace.rs index a86ea0545..6723e4be3 100644 --- a/src/logspace.rs +++ b/src/logspace.rs @@ -65,13 +65,16 @@ where impl ExactSizeIterator for Logspace where Logspace: Iterator {} -/// An iterator of a sequence of logarithmically spaced number. +/// An iterator of a sequence of logarithmically spaced numbers. /// /// The `Logspace` has `n` elements, where the first element is `base.powf(a)` /// and the last element is `base.powf(b)`. If `base` is negative, this /// iterator will return all negative values. /// -/// Iterator element type is `F`, where `F` must be either `f32` or `f64`. +/// The iterator element type is `F`, where `F` must implement `Float`, e.g. +/// `f32` or `f64`. +/// +/// **Panics** if converting `n - 1` to type `F` fails. #[inline] pub fn logspace(base: F, a: F, b: F, n: usize) -> Logspace where