Conversation
- Sampler, pruner, storage, parameter, and multi_objective types are no longer re-exported at the crate root; access via module paths instead (e.g. optimizer::sampler::TpeSampler, optimizer::parameter::FloatParam) - Prelude continues to re-export everything for convenience - Add module-level pub use re-exports in sampler/mod.rs and parameter.rs - Update derive macro to reference optimizer::parameter::Categorical - Rename sampler::differential_evolution to sampler::de - Fix all downstream imports in tests, benches, and doctests - Fix all rustdoc intra-doc links to use explicit module paths
…topic files - Split pruning_and_callbacks into pruning and early_stopping - Split advanced_features into async_parallel, journal_storage, ask_and_tell, multi_objective - Each example now requires only its own feature flag - Trim sampler_comparison winner logic and verbose header - Update CI workflow and README to match new example names
- Add `Objective<V>` trait with lifecycle hooks (`before_trial`, `after_trial`, `max_retries`) in new `src/objective.rs` - Replace 14+ optimize variants with 6 methods: `optimize`, `optimize_with`, and async/parallel counterparts - `optimize*` methods accept closures directly (FnMut for sync, Fn for async); `optimize_with*` methods accept `impl Objective<V>` for struct-based objectives with hooks and retries - Remove `optimize_until`, `optimize_with_callback`, `optimize_with_retries`, `optimize_with_checkpoint`, and all deprecated `_with_sampler` methods
- Delete 20 duplicate tests already covered by parameter_tests.rs - Move 11 pure Trial unit tests into src/trial.rs - Split remaining 84 integration tests into tests/study/ (9 modules) - Group sampler tests into tests/sampler/ (7 modules) - Group pruner tests into tests/pruner/ (2 modules)
There was a problem hiding this comment.
Pull request overview
This PR is a comprehensive refactoring and documentation enhancement effort. The changes include:
Changes:
- Reorganizes test suite into modular structure with dedicated directories for
study/,sampler/, andpruner/tests - Enhances API documentation across all samplers, pruners, and storage modules with detailed descriptions, usage guidelines, and examples
- Introduces breaking API changes: moves
ParamValuetooptimizer::parameter, renamesdifferential_evolutionmodule tode, movesDecompositiontooptimizer::sampler, and simplifies async API - Replaces old examples with new focused examples covering basic optimization, parameter types, sampler comparison, pruning, early stopping, async/parallel execution, journal storage, ask-and-tell, and multi-objective optimization
- Adds project infrastructure:
cliff.tomlfor changelog generation,CLAUDE.mdfor AI guidelines,.claude/commands/for git workflows, andMakefilewith changelog target - Updates
README.mdwith streamlined quick start and feature summary
Reviewed changes
Copilot reviewed 87 out of 91 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/study/* | New modular test organization for study functionality |
| tests/sampler/* | New test files and module structure for samplers |
| tests/pruner/* | New comprehensive pruner test suite |
| tests/*_tests.rs | Updated import paths and async API usage |
| src/sampler/*.rs | Enhanced documentation with algorithm descriptions and configuration tables |
| src/pruner/*.rs | Added comprehensive module and type documentation |
| src/storage/*.rs | Expanded documentation for storage backends |
| src/objective.rs | New file documenting the Objective trait |
| src/*.rs | Documentation improvements across error, param, pareto, fanova, importance, multi_objective |
| examples/*.rs | Replaced verbose examples with focused, runnable examples |
| Cargo.toml | Updated description and example configuration |
| README.md | Streamlined with quick start and feature table |
| .github/workflows/ci.yml | Updated to run new examples and doc tests |
| cliff.toml, CHANGELOG.md, Makefile, CLAUDE.md, .claude/commands/* | New project infrastructure files |
Comments suppressed due to low confidence (1)
tests/sampler/differential_evolution.rs:2
- The import path has changed from
optimizer::sampler::differential_evolutiontooptimizer::sampler::de. All references toDifferentialEvolutionSamplerandDifferentialEvolutionStrategyneed to be updated throughout the codebase to use the new module name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7 +/- ##
==========================================
+ Coverage 91.53% 93.26% +1.72%
==========================================
Files 41 53 +12
Lines 14833 14563 -270
==========================================
+ Hits 13578 13582 +4
+ Misses 1255 981 -274 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark ComparisonBase: Click to expand full results
|
- Add blanket `impl Objective<V> for Fn(&mut Trial) -> Result<V, E>` so closures work directly with `optimize` - Rewrite optimize, optimize_async, optimize_parallel to accept `impl Objective<V>` with before_trial/after_trial hooks - Remove optimize_with, optimize_with_async, optimize_with_parallel - Remove max_retries and retry logic from Objective trait - Add explicit closure type annotations for HRTB inference - Convert FnMut test closures to Fn via RefCell/Cell
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 97 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- GridSearchSampler → GridSampler - DifferentialEvolutionSampler → DESampler - DifferentialEvolutionStrategy → DEStrategy - DifferentialEvolutionSamplerBuilder → DESamplerBuilder
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 97 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 95 out of 97 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 97 out of 113 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| path.file_name().unwrap_or_default().to_string_lossy() | ||
| )); | ||
| let file = std::fs::File::create(&tmp_path)?; | ||
| serde_json::to_writer_pretty(file, &snapshot).map_err(std::io::Error::other)?; |
There was a problem hiding this comment.
std::fs::rename is not a reliable atomic replace on all platforms (notably Windows, where renaming over an existing destination fails). To keep the 'atomic write' guarantee, replace this with a platform-safe atomic replace strategy (e.g., remove existing destination before rename, or use a temp file + persist/replace pattern that overwrites the destination).
| serde_json::to_writer_pretty(file, &snapshot).map_err(std::io::Error::other)?; | |
| serde_json::to_writer_pretty(file, &snapshot).map_err(std::io::Error::other)?; | |
| // On Windows, std::fs::rename fails if the destination already exists. | |
| // Remove the destination file first (if present) to make this replace | |
| // behavior work consistently across platforms. | |
| if path.exists() { | |
| if let Err(e) = std::fs::remove_file(path) { | |
| if e.kind() != std::io::ErrorKind::NotFound { | |
| return Err(e); | |
| } | |
| } | |
| } |
| objective: impl crate::objective::Objective<V>, | ||
| ) -> crate::Result<()> | ||
| where | ||
| V: Clone + Default, |
There was a problem hiding this comment.
optimize adds a V: Default bound but the implementation shown doesn’t use Default. This unnecessarily restricts which objective value types can be optimized and is an API footgun for users with non-Default value types. Consider removing the Default bound (keeping only what’s required by into_completed/storage/etc.).
| V: Clone + Default, | |
| V: Clone, |
| // Collect all unique attribute keys (sorted). | ||
| let mut attr_keys: Vec<String> = Vec::new(); | ||
| for trial in trials.iter() { | ||
| for key in trial.user_attrs.keys() { | ||
| if !attr_keys.contains(key) { | ||
| attr_keys.push(key.clone()); | ||
| } | ||
| } | ||
| } | ||
| attr_keys.sort(); |
There was a problem hiding this comment.
Collecting attr_keys with Vec::contains makes this O(trials × attrs²) in the worst case. Using a BTreeSet<String> (or HashSet + final sort) would make key collection scale much better for large studies while keeping deterministic column order.
| seed: u64, | ||
| /// Monotonic counter to disambiguate calls with identical (`trial_id`, distribution). | ||
| call_seq: AtomicU64, |
There was a problem hiding this comment.
Using a single global call_seq means the RNG stream depends on cross-thread call interleaving. In parallel optimization, this can make seeded runs non-reproducible across executions with the same seed (because seq assignment is scheduling-dependent). If reproducibility under concurrency is a goal, consider deriving the per-sample seed from stable identifiers only (e.g., include ParamId / param label / per-trial suggestion index maintained inside Trial) rather than a global atomic sequence.
| let seq = self.call_seq.fetch_add(1, Ordering::Relaxed); | ||
| let mut rng = fastrand::Rng::with_seed(rng_util::mix_seed( | ||
| self.seed, | ||
| trial_id, | ||
| rng_util::distribution_fingerprint(distribution).wrapping_add(seq), | ||
| )); |
There was a problem hiding this comment.
Using a single global call_seq means the RNG stream depends on cross-thread call interleaving. In parallel optimization, this can make seeded runs non-reproducible across executions with the same seed (because seq assignment is scheduling-dependent). If reproducibility under concurrency is a goal, consider deriving the per-sample seed from stable identifiers only (e.g., include ParamId / param label / per-trial suggestion index maintained inside Trial) rather than a global atomic sequence.
| trials | ||
| .iter() | ||
| .map(|trial| { | ||
| param_order | ||
| .iter() | ||
| .filter_map(|param_id| { | ||
| trial.params.get(param_id).and_then(|value| match value { | ||
| crate::param::ParamValue::Float(f) => Some(*f), | ||
| crate::param::ParamValue::Int(i) => Some(*i as f64), | ||
| crate::param::ParamValue::Categorical(_) => None, // Skip categorical | ||
| }) | ||
| }) | ||
| .collect() | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
Because categorical parameters are skipped via filter_map, each row’s length depends on how many categorical ParamIds are present in param_order. Many multivariate modeling flows expect fixed-width observation vectors aligned to a consistent parameter order. Consider either: (1) ensuring param_order contains only numeric params before calling this, or (2) emitting a fixed-width vector (e.g., placeholder value) so downstream code can rely on consistent dimensionality.
src/study/persistence.rs
Outdated
| pub fn export_json(&self, path: impl AsRef<std::path::Path>) -> std::io::Result<()> { | ||
| let file = std::fs::File::create(path)?; | ||
| let trials = self.trials(); | ||
| serde_json::to_writer_pretty(file, &trials).map_err(std::io::Error::other) | ||
| } |
There was a problem hiding this comment.
The new JSON persistence API (export_json, save, load) introduces important behavior (schema versioning, trial round-tripping, ID continuity) but there are no tests shown here validating round-trip correctness. Adding a test that runs a small study, save()s it, then load()s and asserts trials/direction/best value match would help prevent regressions.
| pub fn save(&self, path: impl AsRef<std::path::Path>) -> std::io::Result<()> { | ||
| let path = path.as_ref(); | ||
| let trials = self.trials(); | ||
| let next_trial_id = trials.iter().map(|t| t.id).max().map_or(0, |id| id + 1); | ||
| let snapshot = StudySnapshot { | ||
| version: 1, | ||
| direction: self.direction, | ||
| trials, | ||
| next_trial_id, | ||
| metadata: HashMap::new(), | ||
| }; |
There was a problem hiding this comment.
The new JSON persistence API (export_json, save, load) introduces important behavior (schema versioning, trial round-tripping, ID continuity) but there are no tests shown here validating round-trip correctness. Adding a test that runs a small study, save()s it, then load()s and asserts trials/direction/best value match would help prevent regressions.
| pub fn load(path: impl AsRef<std::path::Path>) -> std::io::Result<Self> { | ||
| use crate::sampler::random::RandomSampler; | ||
|
|
||
| let file = std::fs::File::open(path)?; | ||
| let snapshot: StudySnapshot<V> = serde_json::from_reader(file) | ||
| .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; | ||
| let storage = crate::storage::MemoryStorage::with_trials(snapshot.trials); | ||
| Ok(Self::with_sampler_and_storage( | ||
| snapshot.direction, | ||
| RandomSampler::new(), | ||
| storage, | ||
| )) | ||
| } |
There was a problem hiding this comment.
The new JSON persistence API (export_json, save, load) introduces important behavior (schema versioning, trial round-tripping, ID continuity) but there are no tests shown here validating round-trip correctness. Adding a test that runs a small study, save()s it, then load()s and asserts trials/direction/best value match would help prevent regressions.
Fix direct path references for in-scope items (GammaStrategy, CompletedTrial) and convert feature-gated items to plain backtick formatting so docs build cleanly without --all-features.
…pers - Remove stale #[allow(dead_code)] and #[allow(too_many_lines)] attributes - Replace #[allow(dead_code)] with #[cfg(test)] for test-only methods - Delete unused fill_remaining_independent() and ParamMeta.dist field - Extract sample_float/int/categorical helpers from TpeSampler, MotpeSampler, and SamplingEngine to shorten match-heavy sample() methods - Extract try_fit_kdes() helper to consolidate repeated fallback blocks - Refactor sample_tpe_float/int to take &FloatDistribution/&IntDistribution instead of destructured args, removing #[allow(too_many_arguments)]
…atch in CompletedTrial::get()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 97 out of 115 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub(crate) fn compute_imputation_value(&self, completed_trials: &[CompletedTrial]) -> f64 { | ||
| match self.constant_liar { | ||
| ConstantLiarStrategy::None => 0.0, // This case is handled before calling this method | ||
| ConstantLiarStrategy::Mean => { | ||
| if completed_trials.is_empty() { | ||
| 0.0 | ||
| } else { | ||
| let sum: f64 = completed_trials.iter().map(|t| t.value).sum(); | ||
| sum / completed_trials.len() as f64 | ||
| } | ||
| } | ||
| ConstantLiarStrategy::Best => { | ||
| // Best means minimum for minimization problems | ||
| completed_trials | ||
| .iter() | ||
| .map(|t| t.value) | ||
| .fold(f64::INFINITY, f64::min) | ||
| } | ||
| ConstantLiarStrategy::Worst => { | ||
| // Worst means maximum for minimization problems | ||
| completed_trials | ||
| .iter() | ||
| .map(|t| t.value) | ||
| .fold(f64::NEG_INFINITY, f64::max) | ||
| } | ||
| ConstantLiarStrategy::Custom(v) => v, | ||
| } | ||
| } |
There was a problem hiding this comment.
ConstantLiarStrategy::Best/Worst return +∞/-∞ when completed_trials is empty. In parallel optimization it’s common to have pending trials before the first completion, and injecting infinite objective values can poison downstream sorting/model fitting. Handle the empty-history case explicitly for Best/Worst (similar to Mean) and return a finite fallback (e.g., 0.0 or a configured default).
| fn sample( | ||
| &self, | ||
| distribution: &Distribution, | ||
| _trial_id: u64, | ||
| trial_id: u64, | ||
| _history: &[CompletedTrial], | ||
| ) -> ParamValue { | ||
| let mut rng = self.rng.lock(); | ||
| let seq = self.call_seq.fetch_add(1, Ordering::Relaxed); | ||
| let mut rng = fastrand::Rng::with_seed(rng_util::mix_seed( | ||
| self.seed, | ||
| trial_id, | ||
| rng_util::distribution_fingerprint(distribution).wrapping_add(seq), | ||
| )); | ||
|
|
||
| match distribution { | ||
| Distribution::Float(d) => { | ||
| let value = if d.log_scale { | ||
| // Sample uniformly in log space | ||
| let log_low = d.low.ln(); | ||
| let log_high = d.high.ln(); | ||
| let log_value = rng_util::f64_range(&mut rng, log_low, log_high); | ||
| log_value.exp() | ||
| } else if let Some(step) = d.step { | ||
| // Sample from step grid | ||
| let n_steps = ((d.high - d.low) / step).floor() as i64; | ||
| let k = rng.i64(0..=n_steps); | ||
| d.low + (k as f64) * step | ||
| } else { | ||
| // Uniform sampling | ||
| rng_util::f64_range(&mut rng, d.low, d.high) | ||
| }; | ||
| ParamValue::Float(value) | ||
| } | ||
| Distribution::Int(d) => { | ||
| let value = if d.log_scale { | ||
| // Sample uniformly in log space, then round | ||
| let log_low = (d.low as f64).ln(); | ||
| let log_high = (d.high as f64).ln(); | ||
| let log_value = rng_util::f64_range(&mut rng, log_low, log_high); | ||
| let raw = log_value.exp().round() as i64; | ||
| // Clamp to bounds since rounding might push outside | ||
| raw.clamp(d.low, d.high) | ||
| } else if let Some(step) = d.step { | ||
| // Sample from step grid | ||
| let n_steps = (d.high - d.low) / step; | ||
| let k = rng.i64(0..=n_steps); | ||
| d.low + k * step | ||
| } else { | ||
| // Uniform sampling | ||
| rng.i64(d.low..=d.high) | ||
| }; | ||
| ParamValue::Int(value) | ||
| } | ||
| Distribution::Categorical(d) => { | ||
| let index = rng.usize(0..d.n_choices); | ||
| ParamValue::Categorical(index) | ||
| } | ||
| } | ||
| super::common::sample_random(&mut rng, distribution) | ||
| } |
There was a problem hiding this comment.
The global call_seq makes sampling order-dependent across threads. In parallel runs, scheduling differences will change seq assignment and therefore produced samples, undermining “with_seed” reproducibility even when trial_id and distributions are identical. Consider removing the global counter from the seed mix and instead deriving any disambiguator deterministically from per-trial information (e.g., include ParamId/a per-trial call index provided by the caller), or accept collisions for identical distributions within a trial rather than introducing cross-thread nondeterminism.
| // Fill in labels from other trials that might have better labels. | ||
| for trial in trials.iter() { | ||
| for (&id, label) in &trial.param_labels { | ||
| param_columns.entry(id).or_insert_with(|| label.clone()); |
There was a problem hiding this comment.
The comment says “fill in labels … that might have better labels”, but or_insert_with never overwrites existing entries (including placeholders like id.to_string()). This can cause CSV headers to remain as numeric IDs even when a better human label exists in later trials. A concrete fix is to overwrite when the existing value is the placeholder (e.g., equals id.to_string()), or to build param_columns primarily from param_labels first and only fall back to id.to_string() at the end.
| param_columns.entry(id).or_insert_with(|| label.clone()); | |
| param_columns | |
| .entry(id) | |
| .and_modify(|existing| { | |
| // Upgrade placeholder labels (e.g., "param_<id>" or `id.to_string()`) | |
| // to a better human-readable label when available. | |
| if *existing == id.to_string() { | |
| *existing = label.clone(); | |
| } | |
| }) | |
| .or_insert_with(|| label.clone()); |
| /// Validates that all floating-point fields are finite (not NaN or | ||
| /// Infinity). | ||
| /// | ||
| /// Checks distribution bounds, parameter values, constraints, and | ||
| /// intermediate values. Returns a description of the first invalid | ||
| /// field found, or `Ok(())` if everything is valid. |
There was a problem hiding this comment.
The docstring says “all floating-point fields”, but the implementation doesn’t validate the trial’s objective value (for CompletedTrial<f64>) and doesn’t validate user_attrs float values (which can also carry NaN/∞). Either tighten the documentation to match what’s checked, or extend validation to include the objective value (when applicable) and float-valued attributes to make the guarantee true.
| /// Validates that all floating-point fields are finite (not NaN or | |
| /// Infinity). | |
| /// | |
| /// Checks distribution bounds, parameter values, constraints, and | |
| /// intermediate values. Returns a description of the first invalid | |
| /// field found, or `Ok(())` if everything is valid. | |
| /// Validates that floating-point fields in distributions, parameters, | |
| /// constraints, and intermediate values are finite (not NaN or Infinity). | |
| /// | |
| /// Checks distribution bounds, parameter values, constraints, and | |
| /// intermediate values. This does not validate the trial's objective | |
| /// value or any float values stored in `user_attrs`. Returns a | |
| /// description of the first invalid field found, or `Ok(())` if | |
| /// everything is valid. |
| pub fn validate(&self) -> core::result::Result<(), String> { | ||
| for (id, dist) in &self.distributions { | ||
| if let Distribution::Float(fd) = dist { | ||
| if !fd.low.is_finite() { | ||
| return Err(format!( | ||
| "trial {}: float distribution for param {id} has non-finite low bound ({})", | ||
| self.id, fd.low | ||
| )); | ||
| } | ||
| if !fd.high.is_finite() { | ||
| return Err(format!( | ||
| "trial {}: float distribution for param {id} has non-finite high bound ({})", | ||
| self.id, fd.high | ||
| )); | ||
| } | ||
| if let Some(step) = fd.step | ||
| && !step.is_finite() | ||
| { | ||
| return Err(format!( | ||
| "trial {}: float distribution for param {id} has non-finite step ({step})", | ||
| self.id | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The docstring says “all floating-point fields”, but the implementation doesn’t validate the trial’s objective value (for CompletedTrial<f64>) and doesn’t validate user_attrs float values (which can also carry NaN/∞). Either tighten the documentation to match what’s checked, or extend validation to include the objective value (when applicable) and float-valued attributes to make the guarantee true.
| //! The output is a single HTML file that can be opened in any browser. | ||
| //! An internet connection is needed on first load to fetch `Plotly.js` | ||
| //! from a CDN. |
There was a problem hiding this comment.
The module docs describe the report as “self-contained” with “embedded Plotly.js”, but later state a network fetch from a CDN is required. Pick one behavior and make the docs consistent (either embed Plotly.js in the HTML, or describe it as a single-file report that loads Plotly.js externally).
| /// # Errors | ||
| /// | ||
| /// Returns an I/O error if the file cannot be created or written. | ||
| pub fn save(&self, path: impl AsRef<std::path::Path>) -> std::io::Result<()> { |
There was a problem hiding this comment.
Study::save/Study::load introduce new persistence behavior (schema/version, atomic write, restore semantics), but there’s no corresponding integration test shown here validating round-trip correctness and error cases (e.g., malformed JSON, missing fields, version mismatch handling). Add a test that saves a study with a few trials, loads it back, and asserts direction/trials/IDs are consistent.
| /// # Errors | ||
| /// | ||
| /// Returns an I/O error if the file cannot be read or parsed. | ||
| pub fn load(path: impl AsRef<std::path::Path>) -> std::io::Result<Self> { |
There was a problem hiding this comment.
Study::save/Study::load introduce new persistence behavior (schema/version, atomic write, restore semantics), but there’s no corresponding integration test shown here validating round-trip correctness and error cases (e.g., malformed JSON, missing fields, version mismatch handling). Add a test that saves a study with a few trials, loads it back, and asserts direction/trials/IDs are consistent.
- 10k sequential trials with RandomSampler - 128 parameters with TPE sampler - 128 concurrent workers with optimize_parallel - 5k trials with TPE and 32 parallel workers
- Timing-based proof that trials run in parallel, not sequentially - Atomic counter proof that multiple workers overlap simultaneously - Panic propagation returns TaskError with panic message - Partial failures with parallel path store only successful trials
…nd pruners - Expand sampler module docs with available samplers tables, custom sampler walkthrough, stateless/stateful patterns, cold start handling, history reading, thread safety, and testing guidance - Expand pruner module docs with stateful/stateless classification, warmup parameters, decorator composition, thread safety, and testing - Add code examples to both trait docs (NoisySampler, StalePruner)
Resolve symlinks and `../` traversals best-effort in `new()` and `open()`, falling back to the original path if canonicalization fails (e.g. file doesn't exist yet).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 116 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…and CSV export - Rename stale "GridSearchSampler" panic message to "GridSampler" - Assert concurrency > 0 in optimize_parallel to prevent deadlock - Fix inaccurate comment in CSV export (empty for all non-complete trials, not just pruned)
* fix: address 18 bugs found during codebase audit High severity: - TPE/MOTPE: match parameters by exact distribution equality instead of flat-mapping over all param values, preventing cross-parameter mixing - MultivariateTpeSampler: find_matching_param now uses search space distributions for exact matching instead of type+range heuristic - JournalStorage: write_to_file no longer advances file_offset (left to refresh), both operations serialized under single io_lock mutex, refresh uses fetch_max and deduplicates by trial ID Medium severity: - NSGA-III: use actual Pareto front ranks for tournament selection instead of artificial cyclic indices - sample_random: apply step quantization after log-scale sampling - internal_bounds: return None for non-positive log-scale bounds - SobolSampler: use per-trial dimension HashMap for concurrent safety - JournalStorage refresh: protect with io_lock mutex, use fetch_max - n_trials(): filter by TrialState::Complete as documented - FloatParam: reject NaN/Infinity in validate() - Pruners: assert n_min_trials >= 1, guard compute_percentile on empty - Visualization: escape_js for importance chart parameter names Low severity: - save(): use peek_next_trial_id() from Storage trait - csv_escape: handle carriage return per RFC 4180 - from_internal: use saturating arithmetic for stepped Int distributions - BoolParam: bounds-check categorical index < 2 - min_max: skip NaN values with safe fallback * ci: trigger CI on pull requests targeting any branch
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 116 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/storage/mod.rs:1
- Adding
peek_next_trial_id()as a required method on the publicStoragetrait is a breaking change for downstream custom storage implementations. If backward compatibility is desired, consider providing a default implementation (even if it returns a conservative/less accurate value) or introducing a separate extension trait used only for persistence. Otherwise, this change should be accompanied by an appropriate semver bump and release notes.
//! Trial storage backends.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn load(path: impl AsRef<std::path::Path>) -> std::io::Result<Self> { | ||
| use crate::sampler::random::RandomSampler; | ||
|
|
||
| let file = std::fs::File::open(path)?; | ||
| let snapshot: StudySnapshot<V> = serde_json::from_reader(file) | ||
| .map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?; | ||
| let storage = crate::storage::MemoryStorage::with_trials(snapshot.trials); | ||
| Ok(Self::with_sampler_and_storage( | ||
| snapshot.direction, | ||
| RandomSampler::new(), | ||
| storage, | ||
| )) | ||
| } |
There was a problem hiding this comment.
StudySnapshot persists next_trial_id, but load() ignores it and reconstructs the counter from max(trial.id)+1 via MemoryStorage::with_trials. This will reuse IDs if the saved study had already allocated IDs for failed trials (or any other non-stored allocation), which can cause ID collisions / inaccurate bookkeeping after reload. Fix by restoring the counter to snapshot.next_trial_id (e.g., add a MemoryStorage::with_trials_and_next_id(...) constructor, or a public/feature-appropriate way to bump/set the counter during load). Also consider validating/handling snapshot.version during load for forward compatibility.
| /// Run async optimization with an objective. | ||
| /// | ||
| /// Like [`optimize`](Self::optimize), but each evaluation is wrapped in | ||
| /// [`spawn_blocking`](tokio::task::spawn_blocking), keeping the async | ||
| /// runtime responsive for CPU-bound objectives. Trials run sequentially. | ||
| /// | ||
| /// Accepts any [`Objective`](crate::Objective) implementation, including | ||
| /// plain closures. Struct-based objectives can override lifecycle hooks. | ||
| /// |
There was a problem hiding this comment.
optimize_async no longer appears to accept an async objective (i.e., a closure returning a Future) and instead always runs a sync Objective::evaluate inside spawn_blocking. If the crate previously supported truly-async objectives (network I/O, etc.), this is a significant behavioral/API shift that may surprise users. Consider either (a) adding a separate API that accepts async objectives (e.g., Fn(Trial) -> Future<Output=...>), or (b) renaming/documenting this more explicitly as an async driver for blocking/CPU-bound work (e.g., optimize_blocking_async) to avoid confusion.
| /// Run async optimization with an objective. | |
| /// | |
| /// Like [`optimize`](Self::optimize), but each evaluation is wrapped in | |
| /// [`spawn_blocking`](tokio::task::spawn_blocking), keeping the async | |
| /// runtime responsive for CPU-bound objectives. Trials run sequentially. | |
| /// | |
| /// Accepts any [`Objective`](crate::Objective) implementation, including | |
| /// plain closures. Struct-based objectives can override lifecycle hooks. | |
| /// | |
| /// Run optimization on an async runtime with a synchronous objective. | |
| /// | |
| /// Like [`optimize`](Self::optimize), but each **synchronous** evaluation is | |
| /// wrapped in [`spawn_blocking`](tokio::task::spawn_blocking), keeping the | |
| /// async runtime responsive for CPU-bound or other blocking objectives. | |
| /// Trials run sequentially. | |
| /// | |
| /// This method does **not** take an async objective (e.g., a closure that | |
| /// returns a `Future`). Instead, it accepts any synchronous | |
| /// [`Objective`](crate::Objective) implementation, including plain closures. | |
| /// Struct-based objectives can override lifecycle hooks. | |
| /// | |
| /// Use this when your objective performs CPU-bound or blocking work that | |
| /// would otherwise block the async runtime. If you need truly async, | |
| /// I/O-bound behavior, keep the objective itself synchronous and have it | |
| /// call into async code via your own coordination, rather than by making | |
| /// the objective return a `Future`. | |
| /// |
| pub async fn optimize_async<O>(&self, n_trials: usize, objective: O) -> crate::Result<()> | ||
| where | ||
| O: crate::objective::Objective<V> + Send + Sync + 'static, |
There was a problem hiding this comment.
optimize_async no longer appears to accept an async objective (i.e., a closure returning a Future) and instead always runs a sync Objective::evaluate inside spawn_blocking. If the crate previously supported truly-async objectives (network I/O, etc.), this is a significant behavioral/API shift that may surprise users. Consider either (a) adding a separate API that accepts async objectives (e.g., Fn(Trial) -> Future<Output=...>), or (b) renaming/documenting this more explicitly as an async driver for blocking/CPU-bound work (e.g., optimize_blocking_async) to avoid confusion.
| /// Internal state for tracking per-trial dimension counters. | ||
| struct SobolState { | ||
| /// The `trial_id` of the current trial (used to reset dimension counter). | ||
| current_trial: u64, | ||
| /// Next Sobol dimension to use for the current trial. | ||
| next_dimension: u32, | ||
| /// Next Sobol dimension for each in-flight trial. | ||
| dimensions: HashMap<u64, u32>, | ||
| } |
There was a problem hiding this comment.
dimensions grows monotonically with trial_id and entries are never removed, so long-running studies will retain one HashMap entry per trial forever. That’s a concrete memory-growth issue. Consider moving the per-trial dimension counter into Trial state (so it drops with the trial), or providing a sampler hook to clear state when a trial completes; if neither is possible, a bounded cache/cleanup strategy (e.g., remove entries once dimension exceeds expected param count, or periodically retain only recent IDs) would mitigate unbounded growth.
| let dimension = state.dimensions.entry(trial_id).or_insert(0); | ||
| let dim = *dimension; | ||
| *dimension = dim + 1; |
There was a problem hiding this comment.
dimensions grows monotonically with trial_id and entries are never removed, so long-running studies will retain one HashMap entry per trial forever. That’s a concrete memory-growth issue. Consider moving the per-trial dimension counter into Trial state (so it drops with the trial), or providing a sampler hook to clear state when a trial completes; if neither is possible, a bounded cache/cleanup strategy (e.g., remove entries once dimension exceeds expected param count, or periodically retain only recent IDs) would mitigate unbounded growth.
| ConstantLiarStrategy::Best => { | ||
| // Best means minimum for minimization problems | ||
| completed_trials | ||
| .iter() | ||
| .map(|t| t.value) | ||
| .fold(f64::INFINITY, f64::min) | ||
| } | ||
| ConstantLiarStrategy::Worst => { | ||
| // Worst means maximum for minimization problems | ||
| completed_trials | ||
| .iter() | ||
| .map(|t| t.value) | ||
| .fold(f64::NEG_INFINITY, f64::max) | ||
| } |
There was a problem hiding this comment.
The constant-liar Best/Worst strategies are hard-coded for minimization (min == best, max == worst). If the sampler supports Direction::Maximize, the imputation step will invert the intended behavior and can skew model fitting during parallel optimization. Fix by selecting min/max based on the study/sampler direction (e.g., store Direction in MultivariateTpeSampler and branch here).
tests/async_tests.rs
Outdated
| let sampler = RandomSampler::with_seed(42); | ||
| let study: Study<f64> = Study::with_sampler(Direction::Minimize, sampler); | ||
|
|
||
| let x_param = FloatParam::new(0.0, 10.0); | ||
|
|
||
| let start = tokio::time::Instant::now(); | ||
| study | ||
| .optimize_parallel(4, 4, move |trial: &mut optimizer::Trial| { | ||
| let x = x_param.suggest(trial)?; | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); | ||
| Ok::<_, Error>(x) | ||
| }) | ||
| .await | ||
| .expect("parallel optimization should succeed"); | ||
|
|
||
| let elapsed = start.elapsed(); | ||
| assert_eq!(study.n_trials(), 4); | ||
| // Sequential would take ~400ms; parallel with concurrency=4 should be ~100ms | ||
| assert!( | ||
| elapsed < std::time::Duration::from_millis(350), | ||
| "expected parallel execution under 350ms, took {elapsed:?}" |
There was a problem hiding this comment.
This test is timing-based and likely to be flaky under CI load or on slower machines (threadpool warmup, contention, noisy neighbors). A more deterministic approach is to assert concurrency via synchronization primitives (e.g., a barrier/latch + atomic max-active assertion), similar to test_parallel_max_concurrency_reached, and avoid hard wall-clock thresholds.
| let sampler = RandomSampler::with_seed(42); | |
| let study: Study<f64> = Study::with_sampler(Direction::Minimize, sampler); | |
| let x_param = FloatParam::new(0.0, 10.0); | |
| let start = tokio::time::Instant::now(); | |
| study | |
| .optimize_parallel(4, 4, move |trial: &mut optimizer::Trial| { | |
| let x = x_param.suggest(trial)?; | |
| std::thread::sleep(std::time::Duration::from_millis(100)); | |
| Ok::<_, Error>(x) | |
| }) | |
| .await | |
| .expect("parallel optimization should succeed"); | |
| let elapsed = start.elapsed(); | |
| assert_eq!(study.n_trials(), 4); | |
| // Sequential would take ~400ms; parallel with concurrency=4 should be ~100ms | |
| assert!( | |
| elapsed < std::time::Duration::from_millis(350), | |
| "expected parallel execution under 350ms, took {elapsed:?}" | |
| use std::sync::atomic::{AtomicUsize, Ordering}; | |
| let sampler = RandomSampler::with_seed(42); | |
| let study: Study<f64> = Study::with_sampler(Direction::Minimize, sampler); | |
| let x_param = FloatParam::new(0.0, 10.0); | |
| let active = Arc::new(AtomicUsize::new(0)); | |
| let max_active = Arc::new(AtomicUsize::new(0)); | |
| let active_c = Arc::clone(&active); | |
| let max_active_c = Arc::clone(&max_active); | |
| study | |
| .optimize_parallel(4, 4, move |trial: &mut optimizer::Trial| { | |
| let x = x_param.suggest(trial)?; | |
| let current = active_c.fetch_add(1, Ordering::SeqCst) + 1; | |
| max_active_c.fetch_max(current, Ordering::SeqCst); | |
| std::thread::sleep(std::time::Duration::from_millis(100)); | |
| active_c.fetch_sub(1, Ordering::SeqCst); | |
| Ok::<_, Error>(x) | |
| }) | |
| .await | |
| .expect("parallel optimization should succeed"); | |
| assert_eq!(study.n_trials(), 4); | |
| let max = max_active.load(Ordering::SeqCst); | |
| assert!( | |
| max >= 2, | |
| "expected at least 2 concurrent workers, but max was {max}" |
- Clamp KDE candidates to parameter bounds before evaluating l(x)/g(x), matching the univariate TPE behavior; without this, candidates scored well at out-of-bounds locations but became suboptimal when clamped - Sort HashMap iterations by ParamId before consuming the seeded RNG to eliminate non-deterministic sampling caused by global ParamId counter - Replace wall-clock timing assertion in async concurrency test with atomic max-active counter to avoid CI flakiness - Gate unused HashSet import behind cfg(feature = "async")
1 participant
No description provided.