Skip to content

Commit 9c3dc5f

Browse files
committed
histogram/strategies: Added lints, minor changes to code
- Added `missing_docs`, `clippy::all`, and `cippy::pedantic` lints - They helped to find certain problems when casting values into other types, none of the warning is relavant and are suppresed and commented - It also warns that the private function `bin_width` doesn't consume the input, so their type signature can be changed to a reference. But actually this is because this function `clone`ed the inputs. This is not a good pattern as it hides the clones. It's better to leave this to the caller and make the clone explicit. So this commit removed the `clone` inside of this function, and documented that the inputs would be consumed.
1 parent a479cba commit 9c3dc5f

File tree

1 file changed

+30
-9
lines changed

1 file changed

+30
-9
lines changed

src/histogram/strategies.rs

+30-9
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@
4444
//! [`Rice`]: struct.Rice.html
4545
//! [`Sqrt`]: struct.Sqrt.html
4646
//! [iqr]: https://www.wikiwand.com/en/Interquartile_range
47+
#![warn(missing_docs, clippy::all, clippy::pedantic)]
48+
4749
use crate::{
4850
histogram::{errors::BinsBuildError, Bins, Edges},
4951
quantile::{interpolate::Nearest, Quantile1dExt, QuantileExt},
@@ -62,6 +64,7 @@ use num_traits::{FromPrimitive, NumOps, Zero};
6264
/// [`GridBuilder`]: ../struct.GridBuilder.html
6365
/// [`Grid`]: ../struct.Grid.html
6466
pub trait BinsBuildingStrategy {
67+
#[allow(missing_docs)]
6568
type Elem: Ord;
6669
/// Returns a strategy that has learnt the required parameter fo building [`Bins`] for given
6770
/// 1-dimensional array, or an `Err` if it is not possible to infer the required parameter
@@ -265,6 +268,11 @@ where
265268
S: Data<Elem = Self::Elem>,
266269
{
267270
let n_elems = a.len();
271+
// casting `n_elems: usize` to `f64` may casus off-by-one error here if `n_elems` > 2 ^ 53,
272+
// but it's not relevant here
273+
#[allow(clippy::cast_precision_loss)]
274+
// casting the rounded square root from `f64` to `usize` is safe
275+
#[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
268276
let n_bins = (n_elems as f64).sqrt().round() as usize;
269277
let min = a.min()?;
270278
let max = a.max()?;
@@ -306,6 +314,11 @@ where
306314
S: Data<Elem = Self::Elem>,
307315
{
308316
let n_elems = a.len();
317+
// casting `n_elems: usize` to `f64` may casus off-by-one error here if `n_elems` > 2 ^ 53,
318+
// but it's not relevant here
319+
#[allow(clippy::cast_precision_loss)]
320+
// casting the rounded cube root from `f64` to `usize` is safe
321+
#[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
309322
let n_bins = (2. * (n_elems as f64).powf(1. / 3.)).round() as usize;
310323
let min = a.min()?;
311324
let max = a.max()?;
@@ -347,6 +360,11 @@ where
347360
S: Data<Elem = Self::Elem>,
348361
{
349362
let n_elems = a.len();
363+
// casting `n_elems: usize` to `f64` may casus off-by-one error here if `n_elems` > 2 ^ 53,
364+
// but it's not relevant here
365+
#[allow(clippy::cast_precision_loss)]
366+
// casting the rounded base-2 log from `f64` to `usize` is safe
367+
#[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
350368
let n_bins = (n_elems as f64).log2().round() as usize + 1;
351369
let min = a.min()?;
352370
let max = a.max()?;
@@ -418,6 +436,9 @@ where
418436
T: Ord + Clone + FromPrimitive + NumOps + Zero,
419437
{
420438
fn compute_bin_width(n_bins: usize, iqr: T) -> T {
439+
// casting `n_bins: usize` to `f64` may casus off-by-one error here if `n_bins` > 2 ^ 53,
440+
// but it's not relevant here
441+
#[allow(clippy::cast_precision_loss)]
421442
let denominator = (n_bins as f64).powf(1. / 3.);
422443
T::from_usize(2).unwrap() * iqr / T::from_f64(denominator).unwrap()
423444
}
@@ -495,8 +516,8 @@ where
495516
}
496517
}
497518

498-
/// Given a range (max, min) and the number of bins, it returns
499-
/// the associated bin_width:
519+
/// Returns the `bin_width`, given the two end points of a range (`max`, `min`), and the number of
520+
/// bins, consuming endpoints
500521
///
501522
/// `bin_width = (max - min)/n`
502523
///
@@ -505,13 +526,13 @@ fn compute_bin_width<T>(min: T, max: T, n_bins: usize) -> T
505526
where
506527
T: Ord + Clone + FromPrimitive + NumOps + Zero,
507528
{
508-
let range = max.clone() - min.clone();
529+
let range = max - min;
509530
range / T::from_usize(n_bins).unwrap()
510531
}
511532

512533
#[cfg(test)]
513534
mod equispaced_tests {
514-
use super::*;
535+
use super::EquiSpaced;
515536

516537
#[test]
517538
fn bin_width_has_to_be_positive() {
@@ -526,7 +547,7 @@ mod equispaced_tests {
526547

527548
#[cfg(test)]
528549
mod sqrt_tests {
529-
use super::*;
550+
use super::{BinsBuildingStrategy, Sqrt};
530551
use ndarray::array;
531552

532553
#[test]
@@ -546,7 +567,7 @@ mod sqrt_tests {
546567

547568
#[cfg(test)]
548569
mod rice_tests {
549-
use super::*;
570+
use super::{BinsBuildingStrategy, Rice};
550571
use ndarray::array;
551572

552573
#[test]
@@ -566,7 +587,7 @@ mod rice_tests {
566587

567588
#[cfg(test)]
568589
mod sturges_tests {
569-
use super::*;
590+
use super::{BinsBuildingStrategy, Sturges};
570591
use ndarray::array;
571592

572593
#[test]
@@ -586,7 +607,7 @@ mod sturges_tests {
586607

587608
#[cfg(test)]
588609
mod fd_tests {
589-
use super::*;
610+
use super::{BinsBuildingStrategy, FreedmanDiaconis};
590611
use ndarray::array;
591612

592613
#[test]
@@ -615,7 +636,7 @@ mod fd_tests {
615636

616637
#[cfg(test)]
617638
mod auto_tests {
618-
use super::*;
639+
use super::{Auto, BinsBuildingStrategy};
619640
use ndarray::array;
620641

621642
#[test]

0 commit comments

Comments
 (0)