From 3898136341b75b4e158ea22defa7d1f5b8fcce26 Mon Sep 17 00:00:00 2001 From: Manuel Raimann Date: Fri, 30 Jan 2026 19:21:35 +0100 Subject: [PATCH] Refactor --- src/error.rs | 23 +++- src/kde.rs | 80 ++++++++------ src/lib.rs | 31 ++++-- src/sampler/mod.rs | 5 +- src/sampler/random.rs | 6 +- src/sampler/tpe.rs | 247 +++++++++++++++++++++++++----------------- src/study.rs | 105 ++++++++++++------ src/trial.rs | 81 ++++++++------ tests/async_tests.rs | 16 ++- tests/integration.rs | 72 +++++++++--- 10 files changed, 433 insertions(+), 233 deletions(-) diff --git a/src/error.rs b/src/error.rs index 7e1e610..d6cff2f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -38,7 +38,28 @@ pub enum TpeError { /// Returned when requesting the best trial but no trials have completed. #[error("no completed trials available")] NoCompletedTrials, + + /// Returned when gamma is not in the valid range (0.0, 1.0). + #[error("invalid gamma: {0} must be in (0.0, 1.0)")] + InvalidGamma(f64), + + /// Returned when bandwidth is not positive. + #[error("invalid bandwidth: {0} must be positive")] + InvalidBandwidth(f64), + + /// Returned when KDE is created with empty samples. + #[error("KDE requires at least one sample")] + EmptySamples, + + /// Returned when an internal invariant is violated. + #[error("internal error: {0}")] + Internal(&'static str), + + /// Returned when an async task fails. + #[cfg(feature = "async")] + #[error("async task error: {0}")] + TaskError(String), } /// A specialized Result type for TPE operations. -pub type Result = std::result::Result; +pub type Result = core::result::Result; diff --git a/src/kde.rs b/src/kde.rs index 1f4e15f..686d9e6 100644 --- a/src/kde.rs +++ b/src/kde.rs @@ -5,6 +5,8 @@ use rand::Rng; +use crate::error::{Result, TpeError}; + /// A Gaussian kernel density estimator for continuous distributions. /// /// KDE estimates a probability density function from a set of samples by @@ -28,7 +30,7 @@ use rand::Rng; /// let sample = kde.sample(&mut rng); /// ``` #[derive(Clone, Debug)] -pub struct KernelDensityEstimator { +pub(crate) struct KernelDensityEstimator { /// The sample points used to construct the KDE. samples: Vec, /// The bandwidth (standard deviation) of the Gaussian kernels. @@ -38,37 +40,45 @@ pub struct KernelDensityEstimator { impl KernelDensityEstimator { /// Creates a new KDE with automatic bandwidth selection using Scott's rule. /// - /// Scott's rule sets bandwidth = n^(-1/5) * std_dev, which works well + /// Scott's rule sets bandwidth = n^(-1/5) * `std_dev`, which works well /// for unimodal distributions close to normal. /// - /// # Panics + /// # Errors /// - /// Panics if `samples` is empty. - pub fn new(samples: Vec) -> Self { - assert!(!samples.is_empty(), "KDE requires at least one sample"); + /// Returns `TpeError::EmptySamples` if `samples` is empty. + pub(crate) fn new(samples: Vec) -> Result { + if samples.is_empty() { + return Err(TpeError::EmptySamples); + } let bandwidth = Self::scotts_rule(&samples); - Self { samples, bandwidth } + Ok(Self { samples, bandwidth }) } /// Creates a new KDE with a specified bandwidth. /// /// Use this when you want explicit control over the smoothing parameter. /// - /// # Panics + /// # Errors /// - /// Panics if `samples` is empty or `bandwidth` is not positive. - pub(crate) fn with_bandwidth(samples: Vec, bandwidth: f64) -> Self { - assert!(!samples.is_empty(), "KDE requires at least one sample"); - assert!(bandwidth > 0.0, "Bandwidth must be positive"); + /// Returns `TpeError::EmptySamples` if `samples` is empty. + /// Returns `TpeError::InvalidBandwidth` if `bandwidth` is not positive. + pub(crate) fn with_bandwidth(samples: Vec, bandwidth: f64) -> Result { + if samples.is_empty() { + return Err(TpeError::EmptySamples); + } + if bandwidth <= 0.0 { + return Err(TpeError::InvalidBandwidth(bandwidth)); + } - Self { samples, bandwidth } + Ok(Self { samples, bandwidth }) } /// Computes bandwidth using Scott's rule. /// /// Scott's rule: h = n^(-1/5) * sigma /// where sigma is the sample standard deviation. + #[allow(clippy::cast_precision_loss)] fn scotts_rule(samples: &[f64]) -> f64 { let n = samples.len() as f64; let std_dev = Self::sample_std_dev(samples); @@ -83,6 +93,7 @@ impl KernelDensityEstimator { } /// Computes the sample standard deviation. + #[allow(clippy::cast_precision_loss)] fn sample_std_dev(samples: &[f64]) -> f64 { let n = samples.len() as f64; let mean = samples.iter().sum::() / n; @@ -95,13 +106,14 @@ impl KernelDensityEstimator { /// The density is computed as the average of Gaussian kernels centered /// at each sample point: /// - /// f(x) = (1/n) * sum_i K((x - x_i) / h) + /// f(x) = (1/n) * `sum_i` K((x - `x_i`) / h) /// /// where K is the standard Gaussian kernel and h is the bandwidth. - pub fn pdf(&self, x: f64) -> f64 { + #[allow(clippy::cast_precision_loss)] + pub(crate) fn pdf(&self, x: f64) -> f64 { let n = self.samples.len() as f64; let inv_bandwidth = 1.0 / self.bandwidth; - let normalization = inv_bandwidth / (2.0 * std::f64::consts::PI).sqrt(); + let normalization = inv_bandwidth / (2.0 * core::f64::consts::PI).sqrt(); let density: f64 = self .samples @@ -120,7 +132,7 @@ impl KernelDensityEstimator { /// Sampling works by: /// 1. Uniformly selecting one of the kernel centers (samples) /// 2. Adding Gaussian noise with the bandwidth as standard deviation - pub fn sample(&self, rng: &mut R) -> f64 { + pub(crate) fn sample(&self, rng: &mut R) -> f64 { // Select a random sample to center the kernel on let idx = rng.random_range(0..self.samples.len()); let center = self.samples[idx]; @@ -130,7 +142,7 @@ impl KernelDensityEstimator { let u1: f64 = rng.random(); let u2: f64 = rng.random(); - let z = (-2.0 * u1.ln()).sqrt() * (2.0 * std::f64::consts::PI * u2).cos(); + let z = (-2.0 * u1.ln()).sqrt() * (2.0 * core::f64::consts::PI * u2).cos(); center + z * self.bandwidth } @@ -148,7 +160,7 @@ mod tests { #[test] fn test_kde_pdf_basic() { let samples = vec![0.0, 1.0, 2.0]; - let kde = KernelDensityEstimator::new(samples); + let kde = KernelDensityEstimator::new(samples).unwrap(); // Density should be positive everywhere assert!(kde.pdf(0.0) > 0.0); @@ -164,17 +176,17 @@ mod tests { #[test] fn test_kde_pdf_integrates_to_one() { let samples = vec![0.0, 1.0, 2.0, 3.0, 4.0]; - let kde = KernelDensityEstimator::new(samples); + let kde = KernelDensityEstimator::new(samples).unwrap(); // Numerical integration over a wide range let n_points = 10000; let low = -10.0; let high = 15.0; - let dx = (high - low) / n_points as f64; + let dx = (high - low) / f64::from(n_points); let integral: f64 = (0..n_points) .map(|i| { - let x = low + (i as f64 + 0.5) * dx; + let x = low + (f64::from(i) + 0.5) * dx; kde.pdf(x) * dx }) .sum(); @@ -189,16 +201,16 @@ mod tests { #[test] fn test_kde_with_bandwidth() { let samples = vec![0.0, 1.0, 2.0]; - let kde = KernelDensityEstimator::with_bandwidth(samples, 0.5); + let kde = KernelDensityEstimator::with_bandwidth(samples, 0.5).unwrap(); - assert_eq!(kde.bandwidth(), 0.5); + assert!((kde.bandwidth() - 0.5).abs() < f64::EPSILON); assert!(kde.pdf(1.0) > 0.0); } #[test] fn test_kde_sample_in_reasonable_range() { let samples = vec![0.0, 1.0, 2.0, 3.0, 4.0]; - let kde = KernelDensityEstimator::new(samples); + let kde = KernelDensityEstimator::new(samples).unwrap(); let mut rng = rand::rng(); // Samples should generally be in a reasonable range around the data @@ -213,7 +225,7 @@ mod tests { #[test] fn test_kde_single_sample() { let samples = vec![5.0]; - let kde = KernelDensityEstimator::new(samples); + let kde = KernelDensityEstimator::new(samples).unwrap(); // Should have positive density near the sample assert!(kde.pdf(5.0) > 0.0); @@ -223,7 +235,7 @@ mod tests { #[test] fn test_kde_identical_samples() { let samples = vec![3.0, 3.0, 3.0, 3.0]; - let kde = KernelDensityEstimator::new(samples); + let kde = KernelDensityEstimator::new(samples).unwrap(); // Should handle degenerate case with identical samples assert!(kde.bandwidth() > 0.0); @@ -233,7 +245,7 @@ mod tests { #[test] fn test_scotts_rule_bandwidth() { let samples = vec![0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0]; - let kde = KernelDensityEstimator::new(samples); + let kde = KernelDensityEstimator::new(samples).unwrap(); // n = 10, n^(-1/5) ≈ 0.631 // std_dev ≈ 2.87 @@ -246,23 +258,23 @@ mod tests { } #[test] - #[should_panic(expected = "KDE requires at least one sample")] fn test_kde_empty_samples() { let samples: Vec = vec![]; - KernelDensityEstimator::new(samples); + let result = KernelDensityEstimator::new(samples); + assert!(matches!(result, Err(TpeError::EmptySamples))); } #[test] - #[should_panic(expected = "Bandwidth must be positive")] fn test_kde_zero_bandwidth() { let samples = vec![1.0, 2.0, 3.0]; - KernelDensityEstimator::with_bandwidth(samples, 0.0); + let result = KernelDensityEstimator::with_bandwidth(samples, 0.0); + assert!(matches!(result, Err(TpeError::InvalidBandwidth(_)))); } #[test] - #[should_panic(expected = "Bandwidth must be positive")] fn test_kde_negative_bandwidth() { let samples = vec![1.0, 2.0, 3.0]; - KernelDensityEstimator::with_bandwidth(samples, -1.0); + let result = KernelDensityEstimator::with_bandwidth(samples, -1.0); + assert!(matches!(result, Err(TpeError::InvalidBandwidth(_)))); } } diff --git a/src/lib.rs b/src/lib.rs index 66ea1b1..8b4c1eb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,14 @@ +#![forbid(unsafe_code)] +#![deny(clippy::all)] +#![deny(unreachable_pub)] +#![deny(clippy::correctness)] +#![deny(clippy::suspicious)] +#![deny(clippy::style)] +#![deny(clippy::complexity)] +#![deny(clippy::perf)] +#![deny(clippy::pedantic)] +#![deny(clippy::std_instead_of_core)] + //! A Tree-Parzen Estimator (TPE) library for black-box optimization. //! //! This library provides an Optuna-like API for hyperparameter optimization @@ -11,10 +22,11 @@ //! # Quick Start //! //! ``` -//! use optimizer::{Direction, Study, TpeSampler}; +//! use optimizer::sampler::tpe::TpeSampler; +//! use optimizer::{Direction, Study}; //! //! // Create a study with TPE sampler -//! let sampler = TpeSampler::builder().seed(42).build(); +//! let sampler = TpeSampler::builder().seed(42).build().unwrap(); //! let study: Study = Study::with_sampler(Direction::Minimize, sampler); //! //! // Optimize x^2 for 20 trials @@ -35,7 +47,9 @@ //! A [`Study`] manages optimization trials. Create one with an optimization direction: //! //! ``` -//! use optimizer::{Direction, RandomSampler, Study, TpeSampler}; +//! use optimizer::sampler::random::RandomSampler; +//! use optimizer::sampler::tpe::TpeSampler; +//! use optimizer::{Direction, Study}; //! //! // Minimize with default random sampler //! let study: Study = Study::new(Direction::Minimize); @@ -79,17 +93,18 @@ //! //! # Configuring TPE //! -//! The [`TpeSampler`] can be configured using the builder pattern: +//! The [`sampler::tpe::TpeSampler`] can be configured using the builder pattern: //! //! ``` -//! use optimizer::TpeSampler; +//! use optimizer::sampler::tpe::TpeSampler; //! //! let sampler = TpeSampler::builder() //! .gamma(0.15) // Quantile for good/bad split //! .n_startup_trials(20) // Random trials before TPE //! .n_ei_candidates(32) // Candidates to evaluate //! .seed(42) // Reproducibility -//! .build(); +//! .build() +//! .unwrap(); //! ``` //! //! # Async and Parallel Optimization @@ -120,13 +135,13 @@ mod distribution; mod error; mod kde; mod param; -mod sampler; +pub mod sampler; mod study; mod trial; mod types; pub use error::{Result, TpeError}; -pub use sampler::{CompletedTrial, RandomSampler, Sampler, TpeSampler, TpeSamplerBuilder}; +pub use param::ParamValue; pub use study::Study; pub use trial::Trial; pub use types::{Direction, TrialState}; diff --git a/src/sampler/mod.rs b/src/sampler/mod.rs index ba2979e..e5e1e0a 100644 --- a/src/sampler/mod.rs +++ b/src/sampler/mod.rs @@ -1,13 +1,10 @@ //! Sampler trait and implementations for parameter sampling. -mod random; +pub mod random; pub mod tpe; use std::collections::HashMap; -pub use random::RandomSampler; -pub use tpe::{TpeSampler, TpeSamplerBuilder}; - use crate::distribution::Distribution; use crate::param::ParamValue; diff --git a/src/sampler/random.rs b/src/sampler/random.rs index 7aee1c5..a59aba7 100644 --- a/src/sampler/random.rs +++ b/src/sampler/random.rs @@ -17,7 +17,7 @@ use crate::sampler::{CompletedTrial, Sampler}; /// # Examples /// /// ``` -/// use optimizer::RandomSampler; +/// use optimizer::sampler::random::RandomSampler; /// /// // Create with default RNG /// let sampler = RandomSampler::new(); @@ -31,6 +31,7 @@ pub struct RandomSampler { impl RandomSampler { /// Creates a new random sampler with a default random seed. + #[must_use] pub fn new() -> Self { Self { rng: Mutex::new(StdRng::from_os_rng()), @@ -40,6 +41,7 @@ impl RandomSampler { /// Creates a new random sampler with a fixed seed for reproducibility. /// /// Using the same seed will produce the same sequence of sampled values. + #[must_use] pub fn with_seed(seed: u64) -> Self { Self { rng: Mutex::new(StdRng::seed_from_u64(seed)), @@ -54,6 +56,7 @@ impl Default for RandomSampler { } impl Sampler for RandomSampler { + #[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss)] fn sample( &self, distribution: &Distribution, @@ -110,6 +113,7 @@ impl Sampler for RandomSampler { } #[cfg(test)] +#[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss)] mod tests { use super::*; use crate::distribution::{CategoricalDistribution, FloatDistribution, IntDistribution}; diff --git a/src/sampler/tpe.rs b/src/sampler/tpe.rs index 9d5a047..7ef57f1 100644 --- a/src/sampler/tpe.rs +++ b/src/sampler/tpe.rs @@ -9,6 +9,7 @@ use rand::rngs::StdRng; use rand::{Rng, SeedableRng}; use crate::distribution::Distribution; +use crate::error::{Result, TpeError}; use crate::kde::KernelDensityEstimator; use crate::param::ParamValue; use crate::sampler::{CompletedTrial, Sampler}; @@ -27,7 +28,7 @@ use crate::sampler::{CompletedTrial, Sampler}; /// # Examples /// /// ``` -/// use optimizer::TpeSampler; +/// use optimizer::sampler::tpe::TpeSampler; /// /// // Create with default settings /// let sampler = TpeSampler::new(); @@ -38,7 +39,8 @@ use crate::sampler::{CompletedTrial, Sampler}; /// .n_startup_trials(20) /// .n_ei_candidates(32) /// .seed(42) -/// .build(); +/// .build() +/// .unwrap(); /// ``` pub struct TpeSampler { /// Fraction of trials to consider as "good" (gamma quantile). @@ -58,9 +60,10 @@ impl TpeSampler { /// /// Default settings: /// - gamma: 0.25 (top 25% of trials are considered "good") - /// - n_startup_trials: 10 (random sampling for first 10 trials) - /// - n_ei_candidates: 24 (evaluate 24 candidates per sample) - /// - kde_bandwidth: None (uses Scott's rule for automatic bandwidth) + /// - `n_startup_trials`: 10 (random sampling for first 10 trials) + /// - `n_ei_candidates`: 24 (evaluate 24 candidates per sample) + /// - `kde_bandwidth`: None (uses Scott's rule for automatic bandwidth) + #[must_use] pub fn new() -> Self { Self { gamma: 0.25, @@ -76,15 +79,17 @@ impl TpeSampler { /// # Examples /// /// ``` - /// use optimizer::TpeSampler; + /// use optimizer::sampler::tpe::TpeSampler; /// /// let sampler = TpeSampler::builder() /// .gamma(0.15) /// .n_startup_trials(20) /// .n_ei_candidates(32) /// .seed(42) - /// .build(); + /// .build() + /// .unwrap(); /// ``` + #[must_use] pub fn builder() -> TpeSamplerBuilder { TpeSamplerBuilder::new() } @@ -99,22 +104,24 @@ impl TpeSampler { /// * `kde_bandwidth` - Optional fixed bandwidth for KDE. If None, uses Scott's rule. /// * `seed` - Optional seed for reproducibility. /// - /// # Panics + /// # Errors /// - /// Panics if gamma is not in (0.0, 1.0) or if kde_bandwidth is Some but not positive. + /// Returns `TpeError::InvalidGamma` if gamma is not in (0.0, 1.0). + /// Returns `TpeError::InvalidBandwidth` if `kde_bandwidth` is Some but not positive. pub fn with_config( gamma: f64, n_startup_trials: usize, n_ei_candidates: usize, kde_bandwidth: Option, seed: Option, - ) -> Self { - assert!( - gamma > 0.0 && gamma < 1.0, - "gamma must be in (0.0, 1.0), got {gamma}" - ); - if let Some(bw) = kde_bandwidth { - assert!(bw > 0.0, "kde_bandwidth must be positive, got {bw}"); + ) -> Result { + if gamma <= 0.0 || gamma >= 1.0 { + return Err(TpeError::InvalidGamma(gamma)); + } + if let Some(bw) = kde_bandwidth + && bw <= 0.0 + { + return Err(TpeError::InvalidBandwidth(bw)); } let rng = match seed { @@ -122,19 +129,24 @@ impl TpeSampler { None => StdRng::from_os_rng(), }; - Self { + Ok(Self { gamma, n_startup_trials, n_ei_candidates, kde_bandwidth, rng: Mutex::new(rng), - } + }) } /// Splits trials into good and bad groups based on the gamma quantile. /// - /// Returns (good_trials, bad_trials) where good_trials contains trials + /// Returns (`good_trials`, `bad_trials`) where `good_trials` contains trials /// with values below the gamma quantile (for minimization). + #[allow( + clippy::cast_precision_loss, + clippy::cast_possible_truncation, + clippy::cast_sign_loss + )] fn split_trials<'a>( &self, history: &'a [CompletedTrial], @@ -149,7 +161,7 @@ impl TpeSampler { history[a] .value .partial_cmp(&history[b].value) - .unwrap_or(std::cmp::Ordering::Equal) + .unwrap_or(core::cmp::Ordering::Equal) }); // Calculate the split point (gamma quantile) @@ -171,6 +183,11 @@ impl TpeSampler { } /// Samples uniformly from a distribution (used during startup phase). + #[allow( + clippy::cast_possible_truncation, + clippy::cast_precision_loss, + clippy::unused_self + )] fn sample_uniform(&self, distribution: &Distribution, rng: &mut StdRng) -> ParamValue { match distribution { Distribution::Float(d) => { @@ -241,6 +258,11 @@ impl TpeSampler { None => KernelDensityEstimator::new(bad_internal), }; + // If KDE construction fails, fall back to uniform sampling + let (Ok(l_kde), Ok(g_kde)) = (l_kde, g_kde) else { + return rng.random_range(low..=high); + }; + // Generate candidates from l(x) and select the one with best l(x)/g(x) ratio let mut best_candidate = internal_low; let mut best_ratio = f64::NEG_INFINITY; @@ -289,15 +311,19 @@ impl TpeSampler { } /// Samples using TPE for integer distributions. - #[allow(clippy::too_many_arguments)] + #[allow( + clippy::too_many_arguments, + clippy::cast_precision_loss, + clippy::cast_possible_truncation + )] fn sample_tpe_int( &self, low: i64, high: i64, log_scale: bool, step: Option, - good_values: Vec, - bad_values: Vec, + good_values: &[i64], + bad_values: &[i64], rng: &mut StdRng, ) -> i64 { // Convert to floats for KDE @@ -331,23 +357,24 @@ impl TpeSampler { } /// Samples using TPE for categorical distributions. + #[allow(clippy::cast_precision_loss, clippy::unused_self)] fn sample_tpe_categorical( &self, n_choices: usize, - good_indices: Vec, - bad_indices: Vec, + good_indices: &[usize], + bad_indices: &[usize], rng: &mut StdRng, ) -> usize { // Count occurrences in good and bad groups let mut good_counts = vec![0usize; n_choices]; let mut bad_counts = vec![0usize; n_choices]; - for &idx in &good_indices { + for &idx in good_indices { if idx < n_choices { good_counts[idx] += 1; } } - for &idx in &bad_indices { + for &idx in bad_indices { if idx < n_choices { bad_counts[idx] += 1; } @@ -395,14 +422,15 @@ impl Default for TpeSampler { /// # Examples /// /// ``` -/// use optimizer::TpeSamplerBuilder; +/// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .gamma(0.15) /// .n_startup_trials(20) /// .n_ei_candidates(32) /// .seed(42) -/// .build(); +/// .build() +/// .unwrap(); /// ``` #[derive(Debug, Clone)] pub struct TpeSamplerBuilder { @@ -418,10 +446,11 @@ impl TpeSamplerBuilder { /// /// Default settings: /// - gamma: 0.25 (top 25% of trials are considered "good") - /// - n_startup_trials: 10 (random sampling for first 10 trials) - /// - n_ei_candidates: 24 (evaluate 24 candidates per sample) - /// - kde_bandwidth: None (uses Scott's rule for automatic bandwidth) + /// - `n_startup_trials`: 10 (random sampling for first 10 trials) + /// - `n_ei_candidates`: 24 (evaluate 24 candidates per sample) + /// - `kde_bandwidth`: None (uses Scott's rule for automatic bandwidth) /// - seed: None (use OS-provided entropy) + #[must_use] pub fn new() -> Self { Self { gamma: 0.25, @@ -441,24 +470,23 @@ impl TpeSamplerBuilder { /// /// * `gamma` - Quantile value, must be in (0.0, 1.0). /// - /// # Panics - /// - /// Panics if gamma is not in (0.0, 1.0). - /// /// # Examples /// /// ``` - /// use optimizer::TpeSamplerBuilder; + /// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .gamma(0.10) // Use top 10% as "good" trials - /// .build(); + /// .build() + /// .unwrap(); /// ``` + /// + /// # Note + /// + /// Validation happens at `build()` time. If gamma is not in (0.0, 1.0), + /// `build()` will return `Err(TpeError::InvalidGamma)`. + #[must_use] pub fn gamma(mut self, gamma: f64) -> Self { - assert!( - gamma > 0.0 && gamma < 1.0, - "gamma must be in (0.0, 1.0), got {gamma}" - ); self.gamma = gamma; self } @@ -476,12 +504,14 @@ impl TpeSamplerBuilder { /// # Examples /// /// ``` - /// use optimizer::TpeSamplerBuilder; + /// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .n_startup_trials(20) // Random sample first 20 trials - /// .build(); + /// .build() + /// .unwrap(); /// ``` + #[must_use] pub fn n_startup_trials(mut self, n: usize) -> Self { self.n_startup_trials = n; self @@ -500,12 +530,14 @@ impl TpeSamplerBuilder { /// # Examples /// /// ``` - /// use optimizer::TpeSamplerBuilder; + /// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .n_ei_candidates(48) // Evaluate more candidates - /// .build(); + /// .build() + /// .unwrap(); /// ``` + #[must_use] pub fn n_ei_candidates(mut self, n: usize) -> Self { self.n_ei_candidates = n; self @@ -523,24 +555,23 @@ impl TpeSamplerBuilder { /// /// * `bandwidth` - The fixed bandwidth (standard deviation) for Gaussian kernels. /// - /// # Panics - /// - /// Panics if bandwidth is not positive. - /// /// # Examples /// /// ``` - /// use optimizer::TpeSamplerBuilder; + /// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .kde_bandwidth(0.5) // Fixed bandwidth of 0.5 - /// .build(); + /// .build() + /// .unwrap(); /// ``` + /// + /// # Note + /// + /// Validation happens at `build()` time. If bandwidth is not positive, + /// `build()` will return `Err(TpeError::InvalidBandwidth)`. + #[must_use] pub fn kde_bandwidth(mut self, bandwidth: f64) -> Self { - assert!( - bandwidth > 0.0, - "kde_bandwidth must be positive, got {bandwidth}" - ); self.kde_bandwidth = Some(bandwidth); self } @@ -554,12 +585,14 @@ impl TpeSamplerBuilder { /// # Examples /// /// ``` - /// use optimizer::TpeSamplerBuilder; + /// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .seed(42) // Reproducible results - /// .build(); + /// .build() + /// .unwrap(); /// ``` + #[must_use] pub fn seed(mut self, seed: u64) -> Self { self.seed = Some(seed); self @@ -567,19 +600,25 @@ impl TpeSamplerBuilder { /// Builds the configured [`TpeSampler`]. /// + /// # Errors + /// + /// Returns `TpeError::InvalidGamma` if gamma is not in (0.0, 1.0). + /// Returns `TpeError::InvalidBandwidth` if `kde_bandwidth` is Some but not positive. + /// /// # Examples /// /// ``` - /// use optimizer::TpeSamplerBuilder; + /// use optimizer::sampler::tpe::TpeSamplerBuilder; /// /// let sampler = TpeSamplerBuilder::new() /// .gamma(0.15) /// .n_startup_trials(20) /// .n_ei_candidates(32) /// .seed(42) - /// .build(); + /// .build() + /// .unwrap(); /// ``` - pub fn build(self) -> TpeSampler { + pub fn build(self) -> Result { TpeSampler::with_config( self.gamma, self.n_startup_trials, @@ -597,6 +636,7 @@ impl Default for TpeSamplerBuilder { } impl Sampler for TpeSampler { + #[allow(clippy::too_many_lines)] fn sample( &self, distribution: &Distribution, @@ -693,8 +733,8 @@ impl Sampler for TpeSampler { d.high, d.log_scale, d.step, - good_values, - bad_values, + &good_values, + &bad_values, &mut rng, ); ParamValue::Int(value) @@ -725,7 +765,7 @@ impl Sampler for TpeSampler { } let index = - self.sample_tpe_categorical(d.n_choices, good_indices, bad_indices, &mut rng); + self.sample_tpe_categorical(d.n_choices, &good_indices, &bad_indices, &mut rng); ParamValue::Categorical(index) } } @@ -733,6 +773,11 @@ impl Sampler for TpeSampler { } #[cfg(test)] +#[allow( + clippy::similar_names, + clippy::cast_sign_loss, + clippy::cast_precision_loss +)] mod tests { use std::collections::HashMap; @@ -756,34 +801,34 @@ mod tests { #[test] fn test_tpe_sampler_new() { let sampler = TpeSampler::new(); - assert_eq!(sampler.gamma, 0.25); + assert!((sampler.gamma - 0.25).abs() < f64::EPSILON); assert_eq!(sampler.n_startup_trials, 10); assert_eq!(sampler.n_ei_candidates, 24); } #[test] fn test_tpe_sampler_with_config() { - let sampler = TpeSampler::with_config(0.15, 20, 32, None, Some(42)); - assert_eq!(sampler.gamma, 0.15); + let sampler = TpeSampler::with_config(0.15, 20, 32, None, Some(42)).unwrap(); + assert!((sampler.gamma - 0.15).abs() < f64::EPSILON); assert_eq!(sampler.n_startup_trials, 20); assert_eq!(sampler.n_ei_candidates, 32); } #[test] - #[should_panic(expected = "gamma must be in (0.0, 1.0)")] fn test_tpe_sampler_invalid_gamma_zero() { - TpeSampler::with_config(0.0, 10, 24, None, None); + let result = TpeSampler::with_config(0.0, 10, 24, None, None); + assert!(matches!(result, Err(TpeError::InvalidGamma(_)))); } #[test] - #[should_panic(expected = "gamma must be in (0.0, 1.0)")] fn test_tpe_sampler_invalid_gamma_one() { - TpeSampler::with_config(1.0, 10, 24, None, None); + let result = TpeSampler::with_config(1.0, 10, 24, None, None); + assert!(matches!(result, Err(TpeError::InvalidGamma(_)))); } #[test] fn test_tpe_startup_random_sampling() { - let sampler = TpeSampler::with_config(0.25, 10, 24, None, Some(42)); + let sampler = TpeSampler::with_config(0.25, 10, 24, None, Some(42)).unwrap(); let dist = Distribution::Float(FloatDistribution { low: 0.0, high: 1.0, @@ -806,7 +851,7 @@ mod tests { #[test] fn test_tpe_split_trials() { - let sampler = TpeSampler::with_config(0.25, 10, 24, None, Some(42)); + let sampler = TpeSampler::with_config(0.25, 10, 24, None, Some(42)).unwrap(); let dist = Distribution::Float(FloatDistribution { low: 0.0, @@ -820,8 +865,8 @@ mod tests { .map(|i| { create_trial( i as u64, - i as f64, - vec![("x", ParamValue::Float(i as f64 / 20.0), dist.clone())], + f64::from(i), + vec![("x", ParamValue::Float(f64::from(i) / 20.0), dist.clone())], ) }) .collect(); @@ -840,7 +885,7 @@ mod tests { #[test] fn test_tpe_samples_float_with_history() { - let sampler = TpeSampler::with_config(0.25, 5, 24, None, Some(42)); + let sampler = TpeSampler::with_config(0.25, 5, 24, None, Some(42)).unwrap(); let dist = Distribution::Float(FloatDistribution { low: 0.0, @@ -852,7 +897,7 @@ mod tests { // Create history where low values (near 0.2) are "good" let history: Vec = (0..20) .map(|i| { - let x = i as f64 / 20.0; + let x = f64::from(i) / 20.0; // Objective is (x - 0.2)^2, minimized at x=0.2 let value = (x - 0.2).powi(2); create_trial( @@ -882,7 +927,7 @@ mod tests { #[test] fn test_tpe_categorical_sampling() { - let sampler = TpeSampler::with_config(0.25, 5, 24, None, Some(42)); + let sampler = TpeSampler::with_config(0.25, 5, 24, None, Some(42)).unwrap(); let dist = Distribution::Categorical(CategoricalDistribution { n_choices: 4 }); @@ -922,7 +967,7 @@ mod tests { #[test] fn test_tpe_int_sampling() { - let sampler = TpeSampler::with_config(0.25, 5, 24, None, Some(42)); + let sampler = TpeSampler::with_config(0.25, 5, 24, None, Some(42)).unwrap(); let dist = Distribution::Int(IntDistribution { low: 0, @@ -968,14 +1013,14 @@ mod tests { .map(|i| { create_trial( i as u64, - i as f64, - vec![("x", ParamValue::Float(i as f64 / 20.0), dist.clone())], + f64::from(i), + vec![("x", ParamValue::Float(f64::from(i) / 20.0), dist.clone())], ) }) .collect(); - let sampler1 = TpeSampler::with_config(0.25, 5, 24, None, Some(12345)); - let sampler2 = TpeSampler::with_config(0.25, 5, 24, None, Some(12345)); + let sampler1 = TpeSampler::with_config(0.25, 5, 24, None, Some(12345)).unwrap(); + let sampler2 = TpeSampler::with_config(0.25, 5, 24, None, Some(12345)).unwrap(); for i in 0..10 { let v1 = sampler1.sample(&dist, i, &history); @@ -987,8 +1032,8 @@ mod tests { #[test] fn test_tpe_sampler_builder_default() { let builder = TpeSamplerBuilder::new(); - let sampler = builder.build(); - assert_eq!(sampler.gamma, 0.25); + let sampler = builder.build().unwrap(); + assert!((sampler.gamma - 0.25).abs() < f64::EPSILON); assert_eq!(sampler.n_startup_trials, 10); assert_eq!(sampler.n_ei_candidates, 24); } @@ -1000,8 +1045,9 @@ mod tests { .n_startup_trials(20) .n_ei_candidates(32) .seed(42) - .build(); - assert_eq!(sampler.gamma, 0.15); + .build() + .unwrap(); + assert!((sampler.gamma - 0.15).abs() < f64::EPSILON); assert_eq!(sampler.n_startup_trials, 20); assert_eq!(sampler.n_ei_candidates, 32); } @@ -1012,8 +1058,9 @@ mod tests { .gamma(0.10) .n_startup_trials(15) .n_ei_candidates(48) - .build(); - assert_eq!(sampler.gamma, 0.10); + .build() + .unwrap(); + assert!((sampler.gamma - 0.10).abs() < f64::EPSILON); assert_eq!(sampler.n_startup_trials, 15); assert_eq!(sampler.n_ei_candidates, 48); } @@ -1021,16 +1068,16 @@ mod tests { #[test] fn test_tpe_sampler_builder_partial() { // Test setting only some options - let sampler = TpeSamplerBuilder::new().gamma(0.20).build(); - assert_eq!(sampler.gamma, 0.20); + let sampler = TpeSamplerBuilder::new().gamma(0.20).build().unwrap(); + assert!((sampler.gamma - 0.20).abs() < f64::EPSILON); assert_eq!(sampler.n_startup_trials, 10); // default assert_eq!(sampler.n_ei_candidates, 24); // default } #[test] - #[should_panic(expected = "gamma must be in (0.0, 1.0)")] fn test_tpe_sampler_builder_invalid_gamma() { - TpeSamplerBuilder::new().gamma(1.5).build(); + let result = TpeSamplerBuilder::new().gamma(1.5).build(); + assert!(matches!(result, Err(TpeError::InvalidGamma(_)))); } #[test] @@ -1042,12 +1089,12 @@ mod tests { step: None, }); - let history: Vec = (0..20) + let history: Vec = (0..20u32) .map(|i| { create_trial( - i as u64, - i as f64, - vec![("x", ParamValue::Float(i as f64 / 20.0), dist.clone())], + u64::from(i), + f64::from(i), + vec![("x", ParamValue::Float(f64::from(i) / 20.0), dist.clone())], ) }) .collect(); @@ -1055,11 +1102,13 @@ mod tests { let sampler1 = TpeSampler::builder() .seed(99999) .n_startup_trials(5) - .build(); + .build() + .unwrap(); let sampler2 = TpeSampler::builder() .seed(99999) .n_startup_trials(5) - .build(); + .build() + .unwrap(); for i in 0..10 { let v1 = sampler1.sample(&dist, i, &history); diff --git a/src/study.rs b/src/study.rs index 5203ab0..4526066 100644 --- a/src/study.rs +++ b/src/study.rs @@ -1,14 +1,15 @@ //! Study implementation for managing optimization trials. #[cfg(feature = "async")] -use std::future::Future; -use std::ops::ControlFlow; +use core::future::Future; +use core::ops::ControlFlow; +use core::sync::atomic::{AtomicU64, Ordering}; use std::sync::Arc; -use std::sync::atomic::{AtomicU64, Ordering}; use parking_lot::RwLock; -use crate::sampler::{CompletedTrial, RandomSampler, Sampler}; +use crate::sampler::random::RandomSampler; +use crate::sampler::{CompletedTrial, Sampler}; use crate::trial::Trial; use crate::types::Direction; @@ -64,6 +65,7 @@ where /// let study: Study = Study::new(Direction::Minimize); /// assert_eq!(study.direction(), Direction::Minimize); /// ``` + #[must_use] pub fn new(direction: Direction) -> Self { Self::with_sampler(direction, RandomSampler::new()) } @@ -78,7 +80,8 @@ where /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// let sampler = RandomSampler::with_seed(42); /// let study: Study = Study::with_sampler(Direction::Maximize, sampler); @@ -107,7 +110,8 @@ where /// # Examples /// /// ``` - /// use optimizer::{Direction, Study, TpeSampler}; + /// use optimizer::sampler::tpe::TpeSampler; + /// use optimizer::{Direction, Study}; /// /// let mut study: Study = Study::new(Direction::Minimize); /// study.set_sampler(TpeSampler::new()); @@ -313,17 +317,15 @@ where match self.direction { Direction::Minimize => { // Reverse ordering: smaller values are "greater" for max_by - ordering - .map(|o| o.reverse()) - .unwrap_or(std::cmp::Ordering::Equal) + ordering.map_or(core::cmp::Ordering::Equal, core::cmp::Ordering::reverse) } Direction::Maximize => { // Normal ordering: larger values are "greater" for max_by - ordering.unwrap_or(std::cmp::Ordering::Equal) + ordering.unwrap_or(core::cmp::Ordering::Equal) } } }) - .expect("trials is not empty"); + .ok_or(crate::TpeError::NoCompletedTrials)?; Ok(best.clone()) } @@ -390,7 +392,8 @@ where /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// // Minimize x^2 /// let sampler = RandomSampler::with_seed(42); @@ -410,7 +413,7 @@ where /// ``` pub fn optimize(&self, n_trials: usize, mut objective: F) -> crate::Result<()> where - F: FnMut(&mut Trial) -> std::result::Result, + F: FnMut(&mut Trial) -> core::result::Result, E: ToString, { for _ in 0..n_trials { @@ -457,7 +460,8 @@ where /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// # #[cfg(feature = "async")] /// # async fn example() -> optimizer::Result<()> { @@ -487,7 +491,7 @@ where ) -> crate::Result<()> where F: Fn(Trial) -> Fut, - Fut: Future>, + Fut: Future>, E: ToString, { for _ in 0..n_trials { @@ -533,11 +537,13 @@ where /// # Errors /// /// Returns `TpeError::NoCompletedTrials` if all trials failed (no successful trials). + /// Returns `TpeError::TaskError` if the semaphore is closed or a spawned task panics. /// /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// # #[cfg(feature = "async")] /// # async fn example() -> optimizer::Result<()> { @@ -568,7 +574,7 @@ where ) -> crate::Result<()> where F: Fn(Trial) -> Fut + Send + Sync + 'static, - Fut: Future> + Send, + Fut: Future> + Send, E: ToString + Send + 'static, V: Send + 'static, { @@ -580,7 +586,11 @@ where let mut handles = Vec::with_capacity(n_trials); for _ in 0..n_trials { - let permit = semaphore.clone().acquire_owned().await.unwrap(); + let permit = semaphore + .clone() + .acquire_owned() + .await + .map_err(|e| crate::TpeError::TaskError(e.to_string()))?; let trial = self.create_trial(); let objective = Arc::clone(&objective); @@ -595,7 +605,10 @@ where // Wait for all tasks and record results for handle in handles { - match handle.await.unwrap() { + match handle + .await + .map_err(|e| crate::TpeError::TaskError(e.to_string()))? + { Ok((trial, value)) => { self.complete_trial(trial, value); } @@ -632,13 +645,15 @@ where /// /// Returns `TpeError::NoCompletedTrials` if no trials completed successfully /// before optimization stopped (either by completing all trials or early stopping). + /// Returns `TpeError::Internal` if a completed trial is not found after adding (internal invariant violation). /// /// # Examples /// /// ``` /// use std::ops::ControlFlow; /// - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// // Stop early when we find a good enough value /// let sampler = RandomSampler::with_seed(42); @@ -673,7 +688,7 @@ where ) -> crate::Result<()> where V: Clone, - F: FnMut(&mut Trial) -> std::result::Result, + F: FnMut(&mut Trial) -> core::result::Result, C: FnMut(&Study, &CompletedTrial) -> ControlFlow<()>, E: ToString, { @@ -686,7 +701,11 @@ where // Get the just-completed trial for the callback let trials = self.completed_trials.read(); - let completed = trials.last().expect("just added a trial"); + let Some(completed) = trials.last() else { + return Err(crate::TpeError::Internal( + "completed trial not found after adding", + )); + }; // Call the callback and check if we should stop // Note: We need to drop the read lock before calling callback @@ -727,7 +746,8 @@ impl Study { /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// // With a seeded sampler for reproducibility /// let sampler = RandomSampler::with_seed(42); @@ -768,7 +788,8 @@ impl Study { /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// // Minimize x^2 with sampler integration /// let sampler = RandomSampler::with_seed(42); @@ -790,7 +811,7 @@ impl Study { mut objective: F, ) -> crate::Result<()> where - F: FnMut(&mut Trial) -> std::result::Result, + F: FnMut(&mut Trial) -> core::result::Result, E: ToString, { for _ in 0..n_trials { @@ -831,13 +852,15 @@ impl Study { /// # Errors /// /// Returns `TpeError::NoCompletedTrials` if no trials completed successfully. + /// Returns `TpeError::Internal` if a completed trial is not found after adding (internal invariant violation). /// /// # Examples /// /// ``` /// use std::ops::ControlFlow; /// - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// // Optimize with sampler integration and early stopping /// let sampler = RandomSampler::with_seed(42); @@ -870,7 +893,7 @@ impl Study { mut callback: C, ) -> crate::Result<()> where - F: FnMut(&mut Trial) -> std::result::Result, + F: FnMut(&mut Trial) -> core::result::Result, C: FnMut(&Study, &CompletedTrial) -> ControlFlow<()>, E: ToString, { @@ -883,7 +906,11 @@ impl Study { // Get the just-completed trial for the callback let trials = self.completed_trials.read(); - let completed = trials.last().expect("just added a trial"); + let Some(completed) = trials.last() else { + return Err(crate::TpeError::Internal( + "completed trial not found after adding", + )); + }; // Call the callback and check if we should stop // Note: We need to drop the read lock before calling callback @@ -931,7 +958,8 @@ impl Study { /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// # #[cfg(feature = "async")] /// # async fn example() -> optimizer::Result<()> { @@ -961,7 +989,7 @@ impl Study { ) -> crate::Result<()> where F: Fn(Trial) -> Fut, - Fut: Future>, + Fut: Future>, E: ToString, { for _ in 0..n_trials { @@ -1007,11 +1035,13 @@ impl Study { /// # Errors /// /// Returns `TpeError::NoCompletedTrials` if all trials failed (no successful trials). + /// Returns `TpeError::TaskError` if the semaphore is closed or a spawned task panics. /// /// # Examples /// /// ``` - /// use optimizer::{Direction, RandomSampler, Study}; + /// use optimizer::sampler::random::RandomSampler; + /// use optimizer::{Direction, Study}; /// /// # #[cfg(feature = "async")] /// # async fn example() -> optimizer::Result<()> { @@ -1042,7 +1072,7 @@ impl Study { ) -> crate::Result<()> where F: Fn(Trial) -> Fut + Send + Sync + 'static, - Fut: Future> + Send, + Fut: Future> + Send, E: ToString + Send + 'static, { use tokio::sync::Semaphore; @@ -1053,7 +1083,11 @@ impl Study { let mut handles = Vec::with_capacity(n_trials); for _ in 0..n_trials { - let permit = semaphore.clone().acquire_owned().await.unwrap(); + let permit = semaphore + .clone() + .acquire_owned() + .await + .map_err(|e| crate::TpeError::TaskError(e.to_string()))?; let trial = self.create_trial_with_sampler(); let objective = Arc::clone(&objective); @@ -1068,7 +1102,10 @@ impl Study { // Wait for all tasks and record results for handle in handles { - match handle.await.unwrap() { + match handle + .await + .map_err(|e| crate::TpeError::TaskError(e.to_string()))? + { Ok((trial, value)) => { self.complete_trial(trial, value); } diff --git a/src/trial.rs b/src/trial.rs index bb1ecda..b53b2d5 100644 --- a/src/trial.rs +++ b/src/trial.rs @@ -37,8 +37,8 @@ pub struct Trial { history: Option>>>>, } -impl std::fmt::Debug for Trial { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl core::fmt::Debug for Trial { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { f.debug_struct("Trial") .field("id", &self.id) .field("state", &self.state) @@ -71,6 +71,7 @@ impl Trial { /// let trial = Trial::new(0); /// assert_eq!(trial.id(), 0); /// ``` + #[must_use] pub fn new(id: u64) -> Self { Self { id, @@ -110,7 +111,7 @@ impl Trial { /// Samples a value from the given distribution using the sampler. /// /// If the trial has a sampler, it delegates to the sampler's sample method - /// with the history of completed trials. Otherwise, it uses the RandomSampler + /// with the history of completed trials. Otherwise, it uses the `RandomSampler` /// as a fallback. fn sample_value(&self, distribution: &Distribution) -> ParamValue { if let (Some(sampler), Some(history)) = (&self.sampler, &self.history) { @@ -118,28 +119,32 @@ impl Trial { sampler.sample(distribution, self.id, &history_guard) } else { // Fallback to RandomSampler when no sampler is configured - use crate::sampler::RandomSampler; + use crate::sampler::random::RandomSampler; let fallback = RandomSampler::new(); fallback.sample(distribution, self.id, &[]) } } /// Returns the unique ID of this trial. + #[must_use] pub fn id(&self) -> u64 { self.id } /// Returns the current state of this trial. + #[must_use] pub fn state(&self) -> TrialState { self.state } /// Returns a reference to the sampled parameters. + #[must_use] pub fn params(&self) -> &HashMap { &self.params } /// Returns a reference to the parameter distributions. + #[must_use] pub fn distributions(&self) -> &HashMap { &self.distributions } @@ -200,8 +205,8 @@ impl Trial { if let Some(existing_dist) = self.distributions.get(&name) { // Verify the distribution matches if let Distribution::Float(existing) = existing_dist - && existing.low == low - && existing.high == high + && (existing.low - low).abs() < f64::EPSILON + && (existing.high - high).abs() < f64::EPSILON && !existing.log_scale && existing.step.is_none() { @@ -220,9 +225,10 @@ impl Trial { // Sample using the sampler let dist = Distribution::Float(distribution); - let value = match self.sample_value(&dist) { - ParamValue::Float(v) => v, - _ => unreachable!("Float distribution should return Float value"), + let ParamValue::Float(value) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Float distribution should return Float value", + )); }; // Store distribution and value @@ -237,7 +243,7 @@ impl Trial { /// The value is sampled uniformly in log space, which is useful for parameters /// that span multiple orders of magnitude (e.g., learning rates). /// - /// If the parameter has already been sampled with the same bounds and log_scale=true, + /// If the parameter has already been sampled with the same bounds and `log_scale=true`, /// the cached value is returned. If the parameter was sampled with different configuration, /// a `ParameterConflict` error is returned. /// @@ -296,8 +302,8 @@ impl Trial { if let Some(existing_dist) = self.distributions.get(&name) { // Verify the distribution matches if let Distribution::Float(existing) = existing_dist - && existing.low == low - && existing.high == high + && (existing.low - low).abs() < f64::EPSILON + && (existing.high - high).abs() < f64::EPSILON && existing.log_scale && existing.step.is_none() { @@ -316,9 +322,10 @@ impl Trial { // Sample using the sampler (sampler handles log-scale transformation) let dist = Distribution::Float(distribution); - let value = match self.sample_value(&dist) { - ParamValue::Float(v) => v, - _ => unreachable!("Float distribution should return Float value"), + let ParamValue::Float(value) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Float distribution should return Float value", + )); }; // Store distribution and value @@ -392,8 +399,8 @@ impl Trial { if let Some(existing_dist) = self.distributions.get(&name) { // Verify the distribution matches if let Distribution::Float(existing) = existing_dist - && existing.low == low - && existing.high == high + && (existing.low - low).abs() < f64::EPSILON + && (existing.high - high).abs() < f64::EPSILON && !existing.log_scale && existing.step == Some(step) { @@ -412,9 +419,10 @@ impl Trial { // Sample using the sampler (sampler handles step-grid) let dist = Distribution::Float(distribution); - let value = match self.sample_value(&dist) { - ParamValue::Float(v) => v, - _ => unreachable!("Float distribution should return Float value"), + let ParamValue::Float(value) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Float distribution should return Float value", + )); }; // Store distribution and value @@ -455,6 +463,7 @@ impl Trial { /// let n2 = trial.suggest_int("n_layers", 1, 10).unwrap(); /// assert_eq!(n, n2); /// ``` + #[allow(clippy::cast_precision_loss)] pub fn suggest_int(&mut self, name: impl Into, low: i64, high: i64) -> Result { if low > high { return Err(TpeError::InvalidBounds { @@ -495,9 +504,10 @@ impl Trial { // Sample using the sampler let dist = Distribution::Int(distribution); - let value = match self.sample_value(&dist) { - ParamValue::Int(v) => v, - _ => unreachable!("Int distribution should return Int value"), + let ParamValue::Int(value) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Int distribution should return Int value", + )); }; // Store distribution and value @@ -512,7 +522,7 @@ impl Trial { /// The value is sampled uniformly in log space, which is useful for parameters /// that span multiple orders of magnitude (e.g., batch sizes). /// - /// If the parameter has already been sampled with the same bounds and log_scale=true, + /// If the parameter has already been sampled with the same bounds and `log_scale=true`, /// the cached value is returned. If the parameter was sampled with different configuration, /// a `ParameterConflict` error is returned. /// @@ -541,6 +551,7 @@ impl Trial { /// let batch_size2 = trial.suggest_int_log("batch_size", 1, 1024).unwrap(); /// assert_eq!(batch_size, batch_size2); /// ``` + #[allow(clippy::cast_precision_loss)] pub fn suggest_int_log(&mut self, name: impl Into, low: i64, high: i64) -> Result { if low < 1 { return Err(TpeError::InvalidLogBounds); @@ -585,9 +596,10 @@ impl Trial { // Sample using the sampler (sampler handles log-scale transformation) let dist = Distribution::Int(distribution); - let value = match self.sample_value(&dist) { - ParamValue::Int(v) => v, - _ => unreachable!("Int distribution should return Int value"), + let ParamValue::Int(value) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Int distribution should return Int value", + )); }; // Store distribution and value @@ -638,6 +650,7 @@ impl Trial { /// .unwrap(); /// assert_eq!(n, n2); /// ``` + #[allow(clippy::cast_precision_loss)] pub fn suggest_int_step( &mut self, name: impl Into, @@ -688,9 +701,10 @@ impl Trial { // Sample using the sampler (sampler handles step-grid) let dist = Distribution::Int(distribution); - let value = match self.sample_value(&dist) { - ParamValue::Int(v) => v, - _ => unreachable!("Int distribution should return Int value"), + let ParamValue::Int(value) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Int distribution should return Int value", + )); }; // Store distribution and value @@ -774,9 +788,10 @@ impl Trial { // Sample using the sampler let dist = Distribution::Categorical(distribution); - let index = match self.sample_value(&dist) { - ParamValue::Categorical(idx) => idx, - _ => unreachable!("Categorical distribution should return Categorical value"), + let ParamValue::Categorical(index) = self.sample_value(&dist) else { + return Err(TpeError::Internal( + "Categorical distribution should return Categorical value", + )); }; // Store distribution and value (store the index) diff --git a/tests/async_tests.rs b/tests/async_tests.rs index e29525b..d046baf 100644 --- a/tests/async_tests.rs +++ b/tests/async_tests.rs @@ -4,7 +4,9 @@ #![cfg(feature = "async")] -use optimizer::{Direction, RandomSampler, Study, TpeError, TpeSampler}; +use optimizer::sampler::random::RandomSampler; +use optimizer::sampler::tpe::TpeSampler; +use optimizer::{Direction, Study, TpeError}; #[tokio::test] async fn test_optimize_async_basic() { @@ -26,7 +28,11 @@ async fn test_optimize_async_basic() { #[tokio::test] async fn test_optimize_async_with_sampler() { - let sampler = TpeSampler::builder().seed(42).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(42) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -61,7 +67,11 @@ async fn test_optimize_parallel() { #[tokio::test] async fn test_optimize_parallel_with_sampler() { - let sampler = TpeSampler::builder().seed(42).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(42) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); diff --git a/tests/integration.rs b/tests/integration.rs index c49cb3c..c576166 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -1,6 +1,14 @@ //! Integration tests for the optimizer library. -use optimizer::{Direction, RandomSampler, Study, TpeError, TpeSampler, Trial}; +#![allow( + clippy::cast_sign_loss, + clippy::cast_precision_loss, + clippy::cast_possible_truncation +)] + +use optimizer::sampler::random::RandomSampler; +use optimizer::sampler::tpe::TpeSampler; +use optimizer::{Direction, Study, TpeError, Trial}; // ============================================================================= // Test: optimize simple quadratic function with TPE, finds near-optimal @@ -14,7 +22,8 @@ fn test_tpe_optimizes_quadratic_function() { .seed(42) .n_startup_trials(5) // Quick startup for test .n_ei_candidates(24) - .build(); + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -40,7 +49,11 @@ fn test_tpe_optimizes_quadratic_function() { fn test_tpe_optimizes_multivariate_function() { // Minimize f(x, y) = x^2 + y^2 where x, y ∈ [-5, 5] // Optimal: (0, 0), f(0, 0) = 0 - let sampler = TpeSampler::builder().seed(123).n_startup_trials(10).build(); + let sampler = TpeSampler::builder() + .seed(123) + .n_startup_trials(10) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -66,7 +79,11 @@ fn test_tpe_optimizes_multivariate_function() { fn test_tpe_maximization() { // Maximize f(x) = -(x - 2)^2 + 10 where x ∈ [-10, 10] // Optimal: x = 2, f(2) = 10 - let sampler = TpeSampler::builder().seed(456).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(456) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Maximize, sampler); @@ -550,7 +567,11 @@ fn test_invalid_step_errors() { #[test] fn test_tpe_with_categorical_parameter() { - let sampler = TpeSampler::builder().seed(42).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(42) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Maximize, sampler); @@ -582,7 +603,11 @@ fn test_tpe_with_categorical_parameter() { #[test] fn test_tpe_with_integer_parameters() { - let sampler = TpeSampler::builder().seed(789).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(789) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -756,7 +781,11 @@ fn test_study_set_sampler() { let mut study: Study = Study::new(Direction::Minimize); // Initially uses RandomSampler, now switch to TPE - let tpe = TpeSampler::builder().seed(42).n_startup_trials(5).build(); + let tpe = TpeSampler::builder() + .seed(42) + .n_startup_trials(5) + .build() + .unwrap(); study.set_sampler(tpe); // Should work with the new sampler @@ -863,10 +892,10 @@ fn test_trial_debug_format() { #[test] fn test_tpe_sampler_builder_default_trait() { - use optimizer::TpeSamplerBuilder; + use optimizer::sampler::tpe::TpeSamplerBuilder; let builder = TpeSamplerBuilder::default(); - let sampler = builder.build(); + let sampler = builder.build().unwrap(); // Should have default values let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -901,7 +930,8 @@ fn test_tpe_with_fixed_kde_bandwidth() { .seed(42) .n_startup_trials(5) .kde_bandwidth(0.5) - .build(); + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -917,9 +947,9 @@ fn test_tpe_with_fixed_kde_bandwidth() { } #[test] -#[should_panic(expected = "kde_bandwidth must be positive")] fn test_tpe_sampler_invalid_kde_bandwidth() { - TpeSampler::with_config(0.25, 10, 24, Some(-1.0), None); + let result = TpeSampler::with_config(0.25, 10, 24, Some(-1.0), None); + assert!(matches!(result, Err(TpeError::InvalidBandwidth(_)))); } #[test] @@ -928,7 +958,8 @@ fn test_tpe_split_trials_with_two_trials() { let sampler = TpeSampler::builder() .seed(42) .n_startup_trials(2) // TPE kicks in after 2 trials - .build(); + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -944,7 +975,11 @@ fn test_tpe_split_trials_with_two_trials() { #[test] fn test_tpe_with_log_scale_int() { - let sampler = TpeSampler::builder().seed(42).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(42) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -962,7 +997,11 @@ fn test_tpe_with_log_scale_int() { #[test] fn test_tpe_with_step_distributions() { - let sampler = TpeSampler::builder().seed(42).n_startup_trials(5).build(); + let sampler = TpeSampler::builder() + .seed(42) + .n_startup_trials(5) + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler); @@ -1040,7 +1079,8 @@ fn test_tpe_empty_good_or_bad_values_fallback() { .seed(42) .n_startup_trials(5) .gamma(0.1) // Very small gamma means few "good" trials - .build(); + .build() + .unwrap(); let study: Study = Study::with_sampler(Direction::Minimize, sampler);