Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Deprecate fixed but unknown #118

wants to merge 5 commits into from

Conversation

mhovd
Copy link
Collaborator

@mhovd mhovd commented Apr 3, 2025

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

mhovd added 4 commits April 3, 2025 12:57
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
@Copilot Copilot bot review requested due to automatic review settings April 3, 2025 11:17
Copy link

@Copilot Copilot AI left a 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);

Copy link
Contributor

github-actions bot commented Apr 3, 2025

🐰 Bencher Report

Branchdeprecate-fixed
Testbedorion
Click to view all benchmark results
BenchmarkLatencyBenchmark 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%)
🐰 View full continuous benchmarking report in Bencher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove support for fixed (but unknown) parameter values
1 participant