fix: address 18 bugs found during codebase audit#8
Merged
Conversation
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
raimannma
force-pushed
the
fix/found-issues
branch
from
0e3750c to
23dfc87
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #8 +/- ##
============================================
- Coverage 93.39% 93.25% -0.15%
============================================
Files 53 53
Lines 14393 14522 +129
============================================
+ Hits 13443 13542 +99
- Misses 950 980 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes 18 bugs identified in a codebase audit, ranging from high-severity correctness issues to low-severity edge cases. The changes primarily address parameter matching in TPE/MOTPE samplers, concurrency issues in JournalStorage, validation gaps, and various other correctness and robustness improvements.
Changes:
- Fixed TPE/MOTPE parameter mixing by using exact Distribution equality instead of flat-mapping values
- Fixed JournalStorage race conditions with unified io_lock and proper deduplication
- Fixed NSGA-III to use actual Pareto front ranks instead of cyclic indices
- Added validation for NaN/Infinity in FloatParam and bounds checking in BoolParam
- Fixed SobolSampler to use per-trial dimension tracking for concurrent safety
- Fixed n_trials() to filter by TrialState::Complete
- Added step quantization after log-scale sampling
- Various other validation and edge case fixes
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/visualization.rs | Apply escape_js to importance chart parameter names; handle NaN values in min_max with safe fallback |
| src/study/persistence.rs | Use peek_next_trial_id() to capture exact counter position including failed trials |
| src/study/mod.rs | Filter n_trials() by TrialState::Complete to exclude pruned/failed trials |
| src/study/export.rs | Add carriage return handling to csv_escape per RFC 4180 |
| src/storage/mod.rs | Add peek_next_trial_id() trait method for persistence |
| src/storage/memory.rs | Implement peek_next_trial_id() with load instead of fetch_add |
| src/storage/journal.rs | Rename write_lock to io_lock; remove file_offset advancement from write_to_file; add deduplication in refresh with fetch_max |
| src/sampler/tpe/sampler.rs | Match parameters by exact Distribution equality via trial.distributions instead of flat-mapping values |
| src/sampler/tpe/multivariate/mod.rs | Use search space distributions for exact matching; cache search space alongside joint sample |
| src/sampler/sobol.rs | Use per-trial dimension HashMap for concurrent safety (note: potential unbounded growth) |
| src/sampler/nsga3.rs | Use actual Pareto front ranks for tournament selection instead of cyclic indices |
| src/sampler/motpe.rs | Match parameters by exact Distribution equality via trial.distributions |
| src/sampler/common.rs | Return None for non-positive log-scale bounds; use saturating arithmetic in from_internal; apply step quantization after log-scale sampling |
| src/pruner/percentile.rs | Assert n_min_trials >= 1; guard compute_percentile on empty input |
| src/pruner/median.rs | Assert n_min_trials >= 1 |
| src/parameter.rs | Reject NaN/Infinity in FloatParam validate(); bounds-check BoolParam categorical index < 2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+56
to
60
| /// 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.
The SobolSampler now uses a per-trial dimension HashMap to track dimensions, but this map grows unboundedly and is never cleaned up. In long-running studies with many trials, this could lead to significant memory usage as completed trial IDs accumulate in the HashMap forever. Consider adding a cleanup mechanism to remove entries for completed trials, or using a bounded cache structure.
Benchmark ComparisonBase: Click to expand full results
|
raimannma
added a commit
that referenced
this pull request
Feb 13, 2026
* 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes all 18 bugs identified in the codebase audit (3 high, 11 medium, 4 low severity).
High severity
sample_float/sample_int/sample_categoricalnow match by exactDistributionequality viat.distributionsinstead of flat-mapping over all param values. Prevents cross-parameter corruption with overlapping ranges.find_matching_param— Uses search space distributions for exact matching instead of type+range heuristic. Cache now stores search space alongside joint sample.write_to_fileno longer advancesfile_offset; bothwrite_to_fileandrefreshserialized under singleio_lockmutex;refreshusesfetch_maxand deduplicates by trial ID.Medium severity
Nonefor non-positive log-scale boundsHashMapfor concurrent safetyTrialState::Completeas documentedvalidate()n_min_trials >= 1, guardcompute_percentileon empty inputescape_jsto importance chart parameter namesLow severity
peek_next_trial_id()fromStoragetraitTest plan
cargo test --all-features)cargo clippy --all-features)cargo fmtclean