Refactor/i64 obj vals#18
Open
znittzel wants to merge 3 commits into
Open
Conversation
Matching clients version to servers version
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the solver API/data model to represent objective values as f64 (instead of i32) to better match solver interfaces and avoid issues with large objective coefficients.
Changes:
- Change
ApiSolution.objective(server) andSolution.objective(Rust client SDK) fromi32tof64. - Update HiGHS and Gurobi solvers to return the full
f64objective instead of rounding/casting toi32, and add new solver tests (GLPK + HiGHS). - Bump server + Rust client versions to
0.2.0and updateglpk-rustdependency to0.1.15.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/models.rs |
Changes API response model objective type to f64. |
src/domain/solvers/highs_solver.rs |
Returns f64 objective; adds tolerance-focused test cases. |
src/domain/solvers/gurobi_solver.rs |
Returns f64 objective instead of rounded i32. |
src/domain/solvers/glpk_solver.rs |
Adds new GLPK solver test coverage around large coefficients/tolerance. |
clients/Rust/src/types.rs |
Updates client SDK solution objective type to f64. |
clients/Rust/Cargo.toml |
Bumps client SDK version to 0.2.0. |
Cargo.toml |
Bumps server version to 0.2.0 and updates glpk-rust. |
Cargo.lock |
Locks updated dependency + package versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
304
to
+308
| .sum(); | ||
|
|
||
| solutions.push(ApiSolution { | ||
| status: api_status, | ||
| objective: objective_value.round() as i32, | ||
| objective: objective_value, |
Comment on lines
289
to
+293
| .sum(); | ||
|
|
||
| solutions.push(ApiSolution { | ||
| status, | ||
| objective: objective_value.round() as i32, | ||
| objective: objective_value, |
| .ok() | ||
| .unwrap(); | ||
| let api_solution = &api_solutions[0]; | ||
| assert_eq!(api_solution.objective, 2f64.powi(EXP_TOL_MAX + 1)); |
Comment on lines
+149
to
+167
| let objectives = vec![HashMap::from([ | ||
| ("x".to_string(), f64::MAX), | ||
| ("y".to_string(), f64::MAX), | ||
| ("z".to_string(), f64::MAX), | ||
| ])]; | ||
| let direction = SolverDirection::Maximize; | ||
| let solutions = solver.solve(&polyhedron, &objectives, direction, false); | ||
| match solutions { | ||
| Ok(solutions) => { | ||
| assert_eq!(solutions.len(), 1); | ||
| let solution = &solutions[0]; | ||
| assert_eq!(solution.error, None); | ||
| println!( | ||
| "Objective value with max coefficients: {}", | ||
| solution.objective | ||
| ); | ||
| // We expect the objective to be inf but still x and y to be 1 | ||
| assert_eq!(solution.objective, f64::INFINITY); | ||
| assert_eq!(solution.solution.get("x"), Some(&1)); |
Comment on lines
+161
to
+164
| println!( | ||
| "Objective value with max coefficients: {}", | ||
| solution.objective | ||
| ); |
| ], | ||
| }; | ||
| // The max exp is one thing in GLPK whereas the tolerance is another. | ||
| // The tolerance seams to be 33 (or 2^(-33)), e.g. the diff between the smallest and largest number used. |
| assert_eq!(api_solution.solution.get("y"), Some(&1)); | ||
| assert_eq!(api_solution.solution.get("z"), Some(&1)); | ||
|
|
||
| // So this test should return a not correct objective value for the solution |
Comment on lines
23
to
25
| pub status: Status, | ||
| pub objective: i32, // matches glpk_rust’s current output | ||
| pub objective: f64, | ||
| pub solution: HashMap<String, i32>, |
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
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.
Description
To mach solvers interface and data model, we change to f64 from i32 for objective value. It arose when problems regarding too large numbers occured.
How Has This Been Tested?
Please describe how this change is tested.
Checklist: