refactor: promote shared primitives to mathematical_optimization#1481
refactor: promote shared primitives to mathematical_optimization#1481mlubin wants to merge 1 commit into
Conversation
febdde9 to
c7d9e22
Compare
|
/ok to test c7d9e22 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (86)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (28)
🚧 Files skipped from review as they are similar to previous changes (54)
📝 WalkthroughWalkthroughThe PR adds ChangesNamespace and utility migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/linear_algebra/vector_math.cuh (1)
100-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
cuda::std::absin the device lambda.The new transform uses unqualified
abs, while the existing device reducer above usescuda::std::abs; making this explicit avoids host/device overload surprises for floating-pointf_t.Proposed fix
- [] __host__ __device__(f_t val) { return abs(val); }, + [] __host__ __device__(f_t val) { return cuda::std::abs(val); },As per coding guidelines, “Prevent numerical instability (overflow, underflow, precision loss) producing wrong results.”
🤖 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/src/linear_algebra/vector_math.cuh` around lines 100 - 113, The device lambda in vector_norm_inf currently uses unqualified abs, which can pick the wrong overload for floating-point device code. Update the transform_reduce lambda to use cuda::std::abs consistently with the rest of the file, and keep the change localized to vector_norm_inf so the maximum absolute value reduction remains explicit and device-safe.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@cpp/src/linear_algebra/vector_math.cuh`:
- Around line 100-113: The device lambda in vector_norm_inf currently uses
unqualified abs, which can pick the wrong overload for floating-point device
code. Update the transform_reduce lambda to use cuda::std::abs consistently with
the rest of the file, and keep the change localized to vector_norm_inf so the
maximum absolute value reduction remains explicit and device-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf69ebbe-a488-4896-8cda-48cf83f0eb13
📒 Files selected for processing (86)
cpp/src/CMakeLists.txtcpp/src/barrier/barrier.cucpp/src/barrier/barrier.hppcpp/src/barrier/conjugate_gradient.hppcpp/src/barrier/cusparse_view.cucpp/src/barrier/cusparse_view.hppcpp/src/barrier/device_sparse_matrix.cucpp/src/barrier/device_sparse_matrix.cuhcpp/src/barrier/iterative_refinement.hppcpp/src/barrier/pinned_host_allocator.cucpp/src/barrier/pinned_host_allocator.hppcpp/src/barrier/sparse_cholesky.cuhcpp/src/barrier/translate_soc.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/deterministic_workers.hppcpp/src/branch_and_bound/diving_heuristics.cppcpp/src/branch_and_bound/mip_node.hppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/branch_and_bound/symmetry.hppcpp/src/branch_and_bound/worker.hppcpp/src/branch_and_bound/worker_pool.hppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hppcpp/src/dual_simplex/CMakeLists.txtcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/basis_updates.hppcpp/src/dual_simplex/bound_flipping_ratio_test.cppcpp/src/dual_simplex/bound_flipping_ratio_test.hppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/crossover.hppcpp/src/dual_simplex/folding.cppcpp/src/dual_simplex/folding.hppcpp/src/dual_simplex/initial_basis.cppcpp/src/dual_simplex/initial_basis.hppcpp/src/dual_simplex/phase1.cppcpp/src/dual_simplex/phase1.hppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/phase2.hppcpp/src/dual_simplex/presolve.cppcpp/src/dual_simplex/presolve.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/primal.hppcpp/src/dual_simplex/right_looking_lu.cppcpp/src/dual_simplex/right_looking_lu.hppcpp/src/dual_simplex/scaling.cppcpp/src/dual_simplex/scaling.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/singletons.cppcpp/src/dual_simplex/singletons.hppcpp/src/dual_simplex/solution.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/dual_simplex/triangle_solve.hppcpp/src/dual_simplex/user_problem.hppcpp/src/linear_algebra/CMakeLists.txtcpp/src/linear_algebra/dense_matrix.hppcpp/src/linear_algebra/dense_vector.hppcpp/src/linear_algebra/sort_csr.cuhcpp/src/linear_algebra/sparse_matrix.cppcpp/src/linear_algebra/sparse_matrix.hppcpp/src/linear_algebra/sparse_vector.cppcpp/src/linear_algebra/sparse_vector.hppcpp/src/linear_algebra/vector_math.cppcpp/src/linear_algebra/vector_math.cuhcpp/src/linear_algebra/vector_math.hppcpp/src/math_optimization/CMakeLists.txtcpp/src/math_optimization/tic_toc.cppcpp/src/math_optimization/tic_toc.hppcpp/src/math_optimization/types.hppcpp/src/mip_heuristics/diversity/lns/rins.cucpp/src/mip_heuristics/diversity/recombiners/sub_mip.cuhcpp/src/mip_heuristics/feasibility_jump/fj_cpu.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/semi_continuous.cucpp/src/mip_heuristics/problem/problem.cucpp/src/mip_heuristics/solve.cucpp/src/pdlp/solve.cucpp/src/pdlp/translate.hppcpp/tests/dual_simplex/unit_tests/right_looking_ldlt.cppcpp/tests/dual_simplex/unit_tests/solve.cppcpp/tests/dual_simplex/unit_tests/solve_barrier.cucpp/tests/mip/cuts_test.cucpp/tests/socp/general_quadratic_test.cu
💤 Files with no reviewable changes (1)
- cpp/src/dual_simplex/CMakeLists.txt
…timization Move general-purpose sparse/dense linear-algebra types and helpers out of the algorithm-specific simplex/barrier/mip namespaces into the parent cuopt::mathematical_optimization namespace, under a new cpp/src/linear_algebra/ directory. These symbols (csc_matrix_t, csr_matrix_t, sparse_vector_t, vector_norm*/dot, matrix(_transpose)_vector_multiply, dense_vector_t, dense_matrix_t, sort_csr) are not tied to any one algorithm and are already consumed across simplex, barrier, pdlp, and mip. sort_csr is additionally decoupled from mip_heuristics by including the public optimization_problem.hpp directly. Explicit template instantiations and out-of-line member definitions for these templates (hosted in barrier/*.cu) are moved into the parent namespace alongside the templates. No behavioral or API change; pure relocation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Miles Lubin <mlubin@nvidia.com>
c7d9e22 to
b132224
Compare
|
/ok to test b132224 |
Moves linear algebra primitives and other shared utilities into the optimization namespace to reduce the need for
usingstatements or namespace qualification introduced by #1446. Linear algebra goes into a new directory namedcpp/src/linear_algebraand other utilities go into the pre-existingcpp/src/math_optimizationdirectory.No behavioral or API change; pure relocation.