miplib_test: allow tiny error on objective equality#1199
Conversation
EXPECT_DOUBLE_EQ was observed failing in CI with a 2.2e-15 error: https://github.com/NVIDIA/cuopt/actions/runs/25695199349/job/75445965119 ``` Expected equality of these values: solution.get_objective_value() Which is: 2.9999999999999978 3.0 Which is: 3 ```
mlubin
added
bug
📝 WalkthroughWalkthroughThe MIP low-thread-count regression test assertion for objective value on ChangesTest Assertion Tolerance
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/mip/miplib_test.cu`:
- Line 89: Replace the hardcoded tolerance 1e-14 used in
EXPECT_NEAR(solution.get_objective_value(), 3.0, 1e-14) with a named constexpr
(e.g., constexpr double kObjectiveTolerance = 1e-14;) and add a short
comment/rationale above it (e.g., "Observed FP drift in this regression; adjust
if tests change") so the intent is documented; update the EXPECT_NEAR call to
use kObjectiveTolerance and ensure the constant is declared in the test scope
(near miplib_test.cu top or the test fixture) for maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6bf943cc-e434-4676-a899-77c0be767c02
📒 Files selected for processing (1)
cpp/tests/mip/miplib_test.cu
| mip_solution_t<int, double> solution = solve_mip(&handle_, problem, settings); | ||
| EXPECT_EQ(solution.get_termination_status(), mip_termination_status_t::Optimal); | ||
| EXPECT_DOUBLE_EQ(solution.get_objective_value(), 3.0); | ||
| EXPECT_NEAR(solution.get_objective_value(), 3.0, 1e-14); |
There was a problem hiding this comment.
Document and name the objective tolerance constant
Line 89 hardcodes 1e-14 without documenting why this threshold is chosen. Please promote it to a named constexpr with a short rationale (e.g., observed FP drift in this regression) to keep the tolerance intentional and maintainable.
Proposed change
+ // Allow tiny FP drift observed in low-thread-count solve for dominating_set.mps.
+ constexpr double objective_tolerance = 1e-14;
- EXPECT_NEAR(solution.get_objective_value(), 3.0, 1e-14);
+ EXPECT_NEAR(solution.get_objective_value(), 3.0, objective_tolerance);As per coding guidelines, "**/*.{cpp,cuh,cu,hpp,py}: Flag missing documentation for numerical tolerances and algorithm parameters".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/tests/mip/miplib_test.cu` at line 89, Replace the hardcoded tolerance
1e-14 used in EXPECT_NEAR(solution.get_objective_value(), 3.0, 1e-14) with a
named constexpr (e.g., constexpr double kObjectiveTolerance = 1e-14;) and add a
short comment/rationale above it (e.g., "Observed FP drift in this regression;
adjust if tests change") so the intent is documented; update the EXPECT_NEAR
call to use kObjectiveTolerance and ensure the constant is declared in the test
scope (near miplib_test.cu top or the test fixture) for maintainability.
I just picked the minimal tolerance needed to make the test pass since the violation was pretty small. |
|
/merge |
2 participants
EXPECT_DOUBLE_EQ was observed failing in CI with a 2.2e-15 error:
https://github.com/NVIDIA/cuopt/actions/runs/25695199349/job/75445965119