-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate fixed but unknown #118
base: main
Are you sure you want to change the base?
Conversation
Instead of Theta holding a wrapper around Parameters, it now holds them directly
Now that Theta holds the parameters, we dont have to throw ranges around everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request removes the support for fixed-but-unknown parameter values (no longer requiring a fixed flag) in order to simplify the API. Key changes include:
- Eliminating the fixed boolean flag from Parameter and related methods.
- Updating all functions, generators, and tests to use the new Parameters API without fixed parameters.
- Removing unused ranges parameters from methods such as suggest_point and check_point.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/onecomp.rs | Updated test to remove fixed parameter input and set progress flag. |
src/structs/theta.rs | Replaced separate random/fixed parameters with a unified parameters field. |
src/routines/settings.rs | Removed fixed flag from Parameter and updated Parameters::add accordingly. |
src/routines/initialization/sobol.rs | Removed filtering based on fixed flag, now using all parameter ranges. |
src/routines/initialization/mod.rs | Simplified Theta construction by using parameters clone. |
src/routines/initialization/latin.rs | Updated Latin hypercube generation to use the new Parameters API. |
src/routines/expansion/adaptative_grid.rs | Adjusted method call to check_point by removing the ranges argument. |
src/algorithms/npod.rs | Removed ranges field and updated suggest_point calls accordingly. |
examples/* and benches/* | Updated examples and benchmarks to reflect the new Parameters API. |
Comments suppressed due to low confidence (2)
src/routines/expansion/adaptative_grid.rs:47
- Ensure that the updated check_point method in Theta correctly retrieves and applies parameter ranges via parameters.ranges(), so that its behavior matches what was previously provided by the explicit ranges argument.
.filter(|point| theta.check_point(point, min_dist))
src/algorithms/npod.rs:361
- Confirm that removing the ranges argument from suggest_point does not adversely affect NPOD's optimization process, and that Theta::check_point uses the correct parameter ranges from the unified Parameters object.
self.theta.suggest_point(cp.to_vec().as_slice(), THETA_D);
|
Branch | deprecate-fixed |
Testbed | orion |
Click to view all benchmark results
Benchmark | Latency | Benchmark Result seconds (s) (Result Δ%) | Upper Boundary seconds (s) (Limit %) |
---|---|---|---|
bimodal_ke_fit | 📈 view plot 🚷 view threshold | 18.29 s(-2.47%)Baseline: 18.75 s | 19.98 s (91.54%) |
As we have discussed, NPAG is not suitable for fixed but unknown parameter values.
As such, this functionality is removed, and as a result the API becomes somewhat cleaner.
This closes #116