Add lp format writer and rename mps references#1567
Conversation
📝 WalkthroughWalkthroughChangesGeneric data model and LP export
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt/cuopt/linear_programming/data_model/data_model_wrapper.pyx (1)
519-527: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject variable-type vectors with inconsistent cardinality.
The helper only handles the empty case; a non-empty
variable_typesarray whose length differs fromn_varsis passed directly to the C++ view. Validate that the vector contains exactly one entry per variable before writing.🤖 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 `@python/cuopt/cuopt/linear_programming/data_model/data_model_wrapper.pyx` around lines 519 - 527, Update _prepare_data_model_view_for_write to validate that a non-empty variable_types array has exactly n_vars entries before calling set_data_model_view. Preserve the existing default creation for an empty vector with variables, and reject mismatched cardinality rather than passing it to the C++ view.
🧹 Nitpick comments (3)
cpp/include/cuopt/mathematical_optimization/io/writer.hpp (1)
26-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
write_lpexecution and failure contract.The new public API documents format support and parameters, but not I/O failure behavior, thread-safety, or GPU/stream requirements. Add the actual contract from the implementation so callers know what errors and execution constraints to expect.
As per path instructions, public C++ headers should document thread-safety, GPU requirements, and numerical behavior.
🤖 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/include/cuopt/mathematical_optimization/io/writer.hpp` around lines 26 - 36, Expand the write_lp documentation to state its I/O failure behavior, thread-safety guarantees or restrictions, GPU/stream requirements, and relevant numerical behavior, using the implementation’s actual contract. Keep the existing format-support and parameter documentation, and anchor the additions to the public write_lp declaration.Source: Path instructions
cpp/include/cuopt/mathematical_optimization/io/lp_writer.hpp (1)
42-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the lifetime requirement on both constructors.
create_view()buildsdata_model_view_tfrom raw pointers into the passed-inproblem's buffers (non-owning view), and the ctor at line 54 stores this inowned_view_. If callers constructlp_writer_tfrom a temporarydata_model_t/data_model_view_tand then callwrite()in a later statement, the referenced data is already destroyed — same hazard as the existingmps_writer_t. Worth a@noteon both ctors that the passed-inproblemmust outlive thelp_writer_tinstance (at least through thewrite()call).📝 Suggested doc addition
/** * `@brief` Ctor. Takes a data model as input and writes it out as an LP formatted file * * `@param`[in] problem Data model to write + * + * `@note` `problem` must outlive this lp_writer_t instance (or at least remain + * valid until write() is called), since the constructed view stores + * non-owning references into `problem`'s underlying buffers. */ lp_writer_t(const data_model_t<i_t, f_t>& problem);As per path instructions: "Suggest documenting thread-safety, GPU requirements, numerical behavior" for new public headers under
cpp/include/cuopt/**/*.🤖 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/include/cuopt/mathematical_optimization/io/lp_writer.hpp` around lines 42 - 54, Add a `@note` to both lp_writer_t constructors documenting that the referenced problem must outlive the lp_writer_t instance, at least until write() completes, because the stored view is non-owning. Apply the same lifetime wording to the data_model_view_t and data_model_t overloads without changing constructor behavior.Source: Path instructions
cpp/include/cuopt/mathematical_optimization/optimization_problem_utils.hpp (1)
38-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the helper to match its generic input.
The changed signature accepts
data_model_t, while the function name and documentation still describe an MPS-only model. Rename it topopulate_from_data_modeland update its call sites so the API reflects the actual contract.🤖 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/include/cuopt/mathematical_optimization/optimization_problem_utils.hpp` around lines 38 - 51, Rename populate_from_mps_data_model to populate_from_data_model, update its documentation to remove MPS-specific wording, and change every call site to use the new name while preserving the existing data_model_t behavior.
🤖 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/include/cuopt/mathematical_optimization/io/data_model.hpp`:
- Around line 331-387: Add a private: access specifier before the model
data-member list containing maximize_, CSR buffers, bounds, metadata, solution
vectors, and quadratic-programming fields. Keep the existing validated accessor
methods public and do not alter field names, initialization, or validation
behavior.
- Line 51: Update the default constructor of data_model_t to initialize
maximize_ explicitly to false, ensuring default-constructed models use
minimization during conversion and writing while preserving existing
initialization behavior for other members.
In `@cpp/include/cuopt/mathematical_optimization/io/utilities/cython_parser.hpp`:
- Around line 17-21: Add a direct `#include` for the standard string header in
cython_parser.hpp before the declarations of call_read and call_parse_mps, so
their std::string parameters do not rely on transitive includes. Preserve the
existing function declarations unchanged.
In `@cpp/include/cuopt/mathematical_optimization/solve.hpp`:
- Around line 72-77: Update all affected solve_lp overload declarations and
Doxygen, including the batch overload, to consistently use data_model_t as the
data-model parameter name instead of mps_data_model or user_problem. Revise each
parameter description to accurately describe the io::data_model_t<i_t, f_t> type
and its actual behavior.
In `@cpp/tests/linear_programming/lp_writer_test.cpp`:
- Around line 149-150: Strengthen the quadratic comparison helper around
has_quadratic_objective() so it validates content rather than only presence and
count. Compare quadratic objective values, indices, and offsets, then inspect
every quadratic constraint’s row metadata, linear terms, RHS, and quadratic
triplets using tolerance for numeric values while preserving exact structural
comparisons.
In `@python/cuopt/cuopt/linear_programming/data_model/data_model.py`:
- Around line 761-764: Add type hints to both new public writeLP methods:
annotate DataModel.writeLP in
python/cuopt/cuopt/linear_programming/data_model/data_model.py at lines 761-764
with user_problem_file: str and a None return type, and annotate the
corresponding writeLP method in python/cuopt/cuopt/linear_programming/problem.py
at lines 2049-2059 with lp_file: str and a None return type.
In `@python/cuopt/cuopt/tests/linear_programming/test_parser.py`:
- Around line 259-304: Expand _assert_models_equivalent to compare
constraint-matrix structure, including column indices/offsets, and assert
get_variable_types() and get_row_names() match between models. Add a separate
test_write_lp_round_trip case using multiple constraints plus Integer and Binary
variables and their bounds sections so the variable-type writer/parser path is
exercised.
---
Outside diff comments:
In `@python/cuopt/cuopt/linear_programming/data_model/data_model_wrapper.pyx`:
- Around line 519-527: Update _prepare_data_model_view_for_write to validate
that a non-empty variable_types array has exactly n_vars entries before calling
set_data_model_view. Preserve the existing default creation for an empty vector
with variables, and reject mismatched cardinality rather than passing it to the
C++ view.
---
Nitpick comments:
In `@cpp/include/cuopt/mathematical_optimization/io/lp_writer.hpp`:
- Around line 42-54: Add a `@note` to both lp_writer_t constructors documenting
that the referenced problem must outlive the lp_writer_t instance, at least
until write() completes, because the stored view is non-owning. Apply the same
lifetime wording to the data_model_view_t and data_model_t overloads without
changing constructor behavior.
In `@cpp/include/cuopt/mathematical_optimization/io/writer.hpp`:
- Around line 26-36: Expand the write_lp documentation to state its I/O failure
behavior, thread-safety guarantees or restrictions, GPU/stream requirements, and
relevant numerical behavior, using the implementation’s actual contract. Keep
the existing format-support and parameter documentation, and anchor the
additions to the public write_lp declaration.
In `@cpp/include/cuopt/mathematical_optimization/optimization_problem_utils.hpp`:
- Around line 38-51: Rename populate_from_mps_data_model to
populate_from_data_model, update its documentation to remove MPS-specific
wording, and change every call site to use the new name while preserving the
existing data_model_t behavior.
🪄 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: 6d5ec415-b588-4d89-8468-00825bcc1b05
📒 Files selected for processing (63)
benchmarks/linear_programming/cuopt/benchmark_helper.hppbenchmarks/linear_programming/cuopt/initial_problem_check.hppbenchmarks/linear_programming/cuopt/run_mip.cppbenchmarks/linear_programming/cuopt/run_pdlp.cucpp/cuopt_cli.cppcpp/include/cuopt/mathematical_optimization/io/data_model.hppcpp/include/cuopt/mathematical_optimization/io/data_model_view.hppcpp/include/cuopt/mathematical_optimization/io/lp_writer.hppcpp/include/cuopt/mathematical_optimization/io/mps_data_model.hppcpp/include/cuopt/mathematical_optimization/io/mps_writer.hppcpp/include/cuopt/mathematical_optimization/io/parser.hppcpp/include/cuopt/mathematical_optimization/io/utilities/cython_parser.hppcpp/include/cuopt/mathematical_optimization/io/writer.hppcpp/include/cuopt/mathematical_optimization/optimization_problem_utils.hppcpp/include/cuopt/mathematical_optimization/solve.hppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/io/CMakeLists.txtcpp/src/io/data_model.cppcpp/src/io/data_model_view.cppcpp/src/io/lp_parser.cppcpp/src/io/lp_parser.hppcpp/src/io/lp_writer.cppcpp/src/io/mps_parser.cppcpp/src/io/mps_parser_internal.hppcpp/src/io/mps_writer.cppcpp/src/io/parser.cppcpp/src/io/utilities/cython_parser.cppcpp/src/io/writer.cppcpp/src/mip_heuristics/solve.cucpp/src/pdlp/cuopt_c.cppcpp/src/pdlp/solve.cucpp/src/pdlp/solve.cuhcpp/src/pdlp/utilities/cython_solve.cucpp/tests/linear_programming/CMakeLists.txtcpp/tests/linear_programming/lp_writer_test.cppcpp/tests/linear_programming/parser_test.cppcpp/tests/linear_programming/pdlp_test.cucpp/tests/linear_programming/unit_tests/optimization_problem_test.cucpp/tests/linear_programming/unit_tests/presolve_test.cucpp/tests/linear_programming/unit_tests/solution_interface_test.cucpp/tests/linear_programming/utilities/pdlp_test_utilities.cuhcpp/tests/mip/bounds_standardization_test.cucpp/tests/mip/cuts_test.cucpp/tests/mip/doc_example_test.cucpp/tests/mip/elim_var_remap_test.cucpp/tests/mip/feasibility_jump_tests.cucpp/tests/mip/incumbent_callback_test.cucpp/tests/mip/load_balancing_test.cucpp/tests/mip/mip_utils.cuhcpp/tests/mip/miplib_test.cucpp/tests/mip/multi_probe_test.cucpp/tests/mip/presolve_test.cucpp/tests/mip/problem_test.cucpp/tests/mip/semi_continuous_test.cucpp/tests/mip/server_test.cucpp/tests/mip/unit_test.cucpp/tests/utilities/inline_lp_test_utils.hppcpp/tests/utilities/inline_mps_test_utils.hpppython/cuopt/cuopt/linear_programming/data_model/data_model.pxdpython/cuopt/cuopt/linear_programming/data_model/data_model.pypython/cuopt/cuopt/linear_programming/data_model/data_model_wrapper.pyxpython/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/tests/linear_programming/test_parser.py
| static_assert(std::is_floating_point<f_t>::value, | ||
| "'data_model_t' accepts only floating point types for weights"); | ||
|
|
||
| data_model_t() = default; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initialize the default optimization sense.
A default-constructed model reads an indeterminate maximize_ value, despite the documented default being minimization. This can select the wrong objective sense during conversion or writing.
Proposed fix
- bool maximize_;
+ bool maximize_{false};Also applies to: 331-332
🤖 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/include/cuopt/mathematical_optimization/io/data_model.hpp` at line 51,
Update the default constructor of data_model_t to initialize maximize_
explicitly to false, ensuring default-constructed models use minimization during
conversion and writing while preserving existing initialization behavior for
other members.
| /** whether to maximize or minimize the objective function */ | ||
| bool maximize_; | ||
| /** | ||
| * the constraint matrix itself in the CSR format | ||
| * @{ | ||
| */ | ||
| std::vector<f_t> A_; | ||
| std::vector<i_t> A_indices_; | ||
| std::vector<i_t> A_offsets_; | ||
| /** @} */ | ||
| /** RHS of the constraints */ | ||
| std::vector<f_t> b_; | ||
| /** weights in the objective function */ | ||
| std::vector<f_t> c_; | ||
| /** scale factor of the objective function */ | ||
| f_t objective_scaling_factor_{1}; | ||
| /** offset of the objective function */ | ||
| f_t objective_offset_{0}; | ||
| /** lower bounds of the variables (primal part) */ | ||
| std::vector<f_t> variable_lower_bounds_; | ||
| /** upper bounds of the variables (primal part) */ | ||
| std::vector<f_t> variable_upper_bounds_; | ||
| /** types of variables can be 'C' or 'I' */ | ||
| std::vector<char> var_types_; | ||
| /** lower bounds of the constraint (dual part) */ | ||
| std::vector<f_t> constraint_lower_bounds_; | ||
| /** upper bounds of the constraint (dual part) */ | ||
| std::vector<f_t> constraint_upper_bounds_; | ||
| /** Type of each constraint */ | ||
| std::vector<char> row_types_; | ||
| /** name of the objective (only a single objective is currently allowed) */ | ||
| std::string objective_name_; | ||
| /** name of the problem */ | ||
| std::string problem_name_; | ||
| /** names of each of the variables in the OP */ | ||
| std::vector<std::string> var_names_{}; | ||
| /** names of linear constraint rows in exported MPS order. */ | ||
| std::vector<std::string> row_names_{}; | ||
| /** number of variables */ | ||
| i_t n_vars_{0}; | ||
| /** number of constraints in the LP representation */ | ||
| i_t n_constraints_{0}; | ||
| /** number of non-zero elements in the constraint matrix */ | ||
| i_t nnz_{0}; | ||
| /** Initial primal solution */ | ||
| std::vector<f_t> initial_primal_solution_; | ||
| /** Initial dual solution */ | ||
| std::vector<f_t> initial_dual_solution_; | ||
|
|
||
| // QPS-specific data members for quadratic programming support | ||
| /** Quadratic objective matrix in CSR format (for (1/2) * x^T * Q * x term) */ | ||
| std::vector<f_t> Q_objective_values_; | ||
| std::vector<i_t> Q_objective_indices_; | ||
| std::vector<i_t> Q_objective_offsets_; | ||
|
|
||
| /** One full quadratic constraint per QCMATRIX block, in order of appearance in the file */ | ||
| std::vector<quadratic_constraint_t> quadratic_constraints_; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the model state private.
These fields are publicly mutable, allowing callers to bypass model invariants and leave dimensions, CSR buffers, and metadata inconsistent. Add a private: section before the field list and retain validated accessors. As per coding guidelines, “keep data members private.”
🤖 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/include/cuopt/mathematical_optimization/io/data_model.hpp` around lines
331 - 387, Add a private: access specifier before the model data-member list
containing maximize_, CSR buffers, bounds, metadata, solution vectors, and
quadratic-programming fields. Keep the existing validated accessor methods
public and do not alter field names, initialization, or validation behavior.
Source: Coding guidelines
| std::unique_ptr<cuopt::mathematical_optimization::io::data_model_t<int, double>> call_read( | ||
| const std::string& file_path, bool fixed_mps_format); | ||
|
|
||
| std::unique_ptr<cuopt::mathematical_optimization::io::mps_data_model_t<int, double>> call_parse_mps( | ||
| std::unique_ptr<cuopt::mathematical_optimization::io::data_model_t<int, double>> call_parse_mps( | ||
| const std::string& mps_file_path, bool fixed_mps_format); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include <string> directly.
This header exposes std::string but relies on a transitive include from data_model.hpp; add #include <string> to keep it self-contained. As per coding guidelines, “C++ headers should be self-contained” and follow Include What You Use.
🤖 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/include/cuopt/mathematical_optimization/io/utilities/cython_parser.hpp`
around lines 17 - 21, Add a direct `#include` for the standard string header in
cython_parser.hpp before the declarations of call_read and call_parse_mps, so
their std::string parameters do not rely on transitive includes. Preserve the
existing function declarations unchanged.
Source: Coding guidelines
| optimization_problem_solution_t<i_t, f_t> solve_lp( | ||
| raft::handle_t const* handle_ptr, | ||
| const cuopt::mathematical_optimization::io::mps_data_model_t<i_t, f_t>& mps_data_model, | ||
| const cuopt::mathematical_optimization::io::data_model_t<i_t, f_t>& mps_data_model, | ||
| pdlp_solver_settings_t<i_t, f_t> const& settings = pdlp_solver_settings_t<i_t, f_t>{}, | ||
| bool problem_checking = true, | ||
| bool use_pdlp_solver_mode = true); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the renamed data-model parameter documentation.
These overloads now accept io::data_model_t, but the declarations and Doxygen still use the MPS-specific name mps_data_model; the batch overload also documents a different parameter name, user_problem. Update the parameter names and descriptions to consistently describe data_model_t<i_t, f_t>.
As per path instructions, C++ header parameter descriptions must match the actual types and behavior.
Also applies to: 107-113, 140-145
🤖 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/include/cuopt/mathematical_optimization/solve.hpp` around lines 72 - 77,
Update all affected solve_lp overload declarations and Doxygen, including the
batch overload, to consistently use data_model_t as the data-model parameter
name instead of mps_data_model or user_problem. Revise each parameter
description to accurately describe the io::data_model_t<i_t, f_t> type and its
actual behavior.
Source: Path instructions
| EXPECT_EQ(a.has_quadratic_objective(), b.has_quadratic_objective()); | ||
| EXPECT_EQ(a.get_quadratic_constraints().size(), b.get_quadratic_constraints().size()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Validate quadratic content, not only its presence.
The quadratic tests pass when coefficients, CSR/COO structure, quadratic-row metadata, linear terms, or RHS values change, because this helper only compares a boolean and a count. Compare the quadratic objective values/indices/offsets and every quadratic constraint’s row metadata, linear terms, RHS, and quadratic triplets with tolerance.
🤖 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/linear_programming/lp_writer_test.cpp` around lines 149 - 150,
Strengthen the quadratic comparison helper around has_quadratic_objective() so
it validates content rather than only presence and count. Compare quadratic
objective values, indices, and offsets, then inspect every quadratic
constraint’s row metadata, linear terms, RHS, and quadratic triplets using
tolerance for numeric values while preserving exact structural comparisons.
Source: Path instructions
|
|
||
| @catch_cuopt_exception | ||
| def writeLP(self, user_problem_file): | ||
| return super().writeLP(user_problem_file) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add type hints to the new writeLP methods.
Both new public writeLP methods were added without type hints, copying the untyped legacy writeMPS signature. Per path instructions, new public Python functions should carry type hints even where older code in the same file doesn't.
python/cuopt/cuopt/linear_programming/data_model/data_model.py#L761-L764: annotate asdef writeLP(self, user_problem_file: str) -> None:.python/cuopt/cuopt/linear_programming/problem.py#L2049-L2059: annotate asdef writeLP(self, lp_file: str) -> None:.
As per path instructions: "Type hints on NEW public functions/classes (do not require them on existing code; there is no mypy config and the codebase is mixed)."
📍 Affects 2 files
python/cuopt/cuopt/linear_programming/data_model/data_model.py#L761-L764(this comment)python/cuopt/cuopt/linear_programming/problem.py#L2049-L2059
🤖 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 `@python/cuopt/cuopt/linear_programming/data_model/data_model.py` around lines
761 - 764, Add type hints to both new public writeLP methods: annotate
DataModel.writeLP in
python/cuopt/cuopt/linear_programming/data_model/data_model.py at lines 761-764
with user_problem_file: str and a None return type, and annotate the
corresponding writeLP method in python/cuopt/cuopt/linear_programming/problem.py
at lines 2049-2059 with lp_file: str and a None return type.
Source: Path instructions
| def _assert_models_equivalent(expected, actual): | ||
| assert actual.get_sense() == expected.get_sense() | ||
| assert ( | ||
| actual.get_variable_names().tolist() | ||
| == expected.get_variable_names().tolist() | ||
| ) | ||
| assert actual.get_objective_coefficients().tolist() == pytest.approx( | ||
| expected.get_objective_coefficients().tolist() | ||
| ) | ||
| assert actual.get_variable_lower_bounds().tolist() == pytest.approx( | ||
| expected.get_variable_lower_bounds().tolist() | ||
| ) | ||
| assert actual.get_variable_upper_bounds().tolist() == pytest.approx( | ||
| expected.get_variable_upper_bounds().tolist() | ||
| ) | ||
| assert actual.get_constraint_matrix_values().tolist() == pytest.approx( | ||
| expected.get_constraint_matrix_values().tolist() | ||
| ) | ||
| assert actual.get_constraint_lower_bounds().tolist() == pytest.approx( | ||
| expected.get_constraint_lower_bounds().tolist() | ||
| ) | ||
| assert actual.get_constraint_upper_bounds().tolist() == pytest.approx( | ||
| expected.get_constraint_upper_bounds().tolist() | ||
| ) | ||
|
|
||
|
|
||
| def test_write_lp_round_trip(): | ||
| # Parse a small LP, write it back out through the DataModel LP writer | ||
| # binding (writeLP -> write_lp), then re-read the produced file and check | ||
| # the model survives the round trip. | ||
| with tempfile.NamedTemporaryFile( | ||
| suffix=".lp", mode="w", delete=False | ||
| ) as f: | ||
| f.write(_MINIMAL_LP) | ||
| src_path = f.name | ||
| with tempfile.NamedTemporaryFile(suffix=".lp", delete=False) as f: | ||
| out_path = f.name | ||
| try: | ||
| src_model = Read(src_path) | ||
| src_model.writeLP(out_path) | ||
| rt_model = Read(out_path) | ||
| finally: | ||
| os.unlink(src_path) | ||
| os.unlink(out_path) | ||
|
|
||
| _assert_models_equivalent(src_model, rt_model) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Round-trip equivalence check omits structure and variable types; test data never exercises integer variables.
_assert_models_equivalent compares objective/bounds/constraint values but never the constraint matrix's column indices/offsets, nor get_variable_types()/get_row_names(). A writer/parser bug that scrambles sparsity structure or drops the LP Integer/Binary section — the "variable-type sections" this PR specifically adds — could pass silently. Compounding this, test_write_lp_round_trip only feeds _MINIMAL_LP (single continuous variable, single constraint), so the variable-type path is never even exercised.
🧪 Proposed fix: add structural/type assertions and an integer-variable case
def _assert_models_equivalent(expected, actual):
assert actual.get_sense() == expected.get_sense()
assert (
actual.get_variable_names().tolist()
== expected.get_variable_names().tolist()
)
+ assert (
+ actual.get_variable_types().tolist()
+ == expected.get_variable_types().tolist()
+ )
+ assert (
+ actual.get_row_names().tolist() == expected.get_row_names().tolist()
+ )
assert actual.get_objective_coefficients().tolist() == pytest.approx(
expected.get_objective_coefficients().tolist()
)
assert actual.get_variable_lower_bounds().tolist() == pytest.approx(
expected.get_variable_lower_bounds().tolist()
)
assert actual.get_variable_upper_bounds().tolist() == pytest.approx(
expected.get_variable_upper_bounds().tolist()
)
+ assert (
+ actual.get_constraint_matrix_indices().tolist()
+ == expected.get_constraint_matrix_indices().tolist()
+ )
+ assert (
+ actual.get_constraint_matrix_offsets().tolist()
+ == expected.get_constraint_matrix_offsets().tolist()
+ )
assert actual.get_constraint_matrix_values().tolist() == pytest.approx(
expected.get_constraint_matrix_values().tolist()
)
assert actual.get_constraint_lower_bounds().tolist() == pytest.approx(
expected.get_constraint_lower_bounds().tolist()
)
assert actual.get_constraint_upper_bounds().tolist() == pytest.approx(
expected.get_constraint_upper_bounds().tolist()
)Also add a second round-trip case with an Integer/Binary bounds section (and ideally multiple constraints) to actually exercise the variable-type writer path.
Based on path instructions, per the review guide's test-quality rule: "new round-trip/parse→write→parse tests must validate numerical/structural equivalence and edge cases ... not just 'runs without error'".
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_parser.py` around lines 259
- 304, Expand _assert_models_equivalent to compare constraint-matrix structure,
including column indices/offsets, and assert get_variable_types() and
get_row_names() match between models. Add a separate test_write_lp_round_trip
case using multiple constraints plus Integer and Binary variables and their
bounds sections so the variable-type writer/parser path is exercised.
Source: Path instructions
mlubin
left a comment
mlubin
left a comment
There was a problem hiding this comment.
Thanks for doing this! I'd prefer if the mps_data_model_t -> data_model_t rename was done in a separate PR to keep the review of the LP writer more focused.
| /** | ||
| * @brief Writes the problem to an LP formatted file | ||
| * | ||
| * Emits the (algebraic, CPLEX/Gurobi style) LP format understood by read_lp(). |
There was a problem hiding this comment.
| * Emits the (algebraic, CPLEX/Gurobi style) LP format understood by read_lp(). | |
| * Emits the LP format understood by read_lp(). |
| const auto& quadratic_constraints = problem_.get_quadratic_constraints(); | ||
|
|
||
| i_t n_variables = 0; | ||
| auto grow = [&](size_t s) { n_variables = std::max(n_variables, static_cast<i_t>(s)); }; |
There was a problem hiding this comment.
Please drop these static_casts, they're not needed. Also it would be good to know why the agents weren't influenced by #1505.
| auto grow = [&](size_t s) { n_variables = std::max(n_variables, static_cast<i_t>(s)); }; | ||
| grow(c_span.size()); | ||
| grow(lb_span.size()); | ||
| grow(ub_span.size()); |
There was a problem hiding this comment.
There's a lot of maneuvering here to accept arrays of different lengths. What if we assert that the arrays all have the same lengths to simplify the code.
| const std::vector<std::pair<i_t, f_t>>& row, | ||
| const char* rel, | ||
| f_t rhs) { | ||
| lp_file << " " << name << ":"; |
There was a problem hiding this comment.
Row names are not required, so if we don't have one we shouldn't print one.
| /* clang-format on */ | ||
|
|
||
| /** | ||
| * Round-trip tests for the LP writer. Each test parses LP text into a |
There was a problem hiding this comment.
It's ok to have a couple round-trip tests but this shouldn't be the primary way we test the writer. We should have a range of input problems where we test the exact strings outputted by the writer, not equivalence. The input problems themselves could be specified in LP format just for convenience.
| ) | ||
|
|
||
|
|
||
| def test_write_lp_round_trip(): |
There was a problem hiding this comment.
Instead of a round-trip test, construct a problem in the python interface and check the precise string that is written.
It might also be useful to consider other names over |
|
Yes, or we could leave mps_data_model_t as is until we make a decision on how to consolidate the problem representations. |
3 participants
Description
Summary
Adds an LP-format writer to cuOpt (complementing the existing MPS writer),
and — since the writer surfaced a long-standing naming issue — renames the
shared, format-neutral problem model from
mps_data_model_ttodata_model_t(the type is used by both the LP and MPS readers/writers and is not
MPS-specific). The LP writer is exposed through the C, C++, and Python APIs.
Changes
LP writer (C++)
lp_writer_t(cpp/src/io/lp_writer.cpp,cpp/include/cuopt/mathematical_optimization/io/lp_writer.hpp), mirroring the MPS writer: objective (incl. quadratic[ ... ] / 2), linear + quadratic constraints, range-constraint splitting, variable bounds, andGenerals/Binaries/Semi-Continuoussections.write_lp(...)added toio/writer.hpp+src/io/writer.cpp.cpp/src/io/CMakeLists.txt.Rename:
mps_data_model_t→data_model_tgit mvio/mps_data_model.hpp→io/data_model.hppandsrc/io/mps_data_model.cpp→src/io/data_model.cpp; class renamed.solve/pdlp/mip, C API, CLI, tests, benchmarks).Rename:
mps_data_model_to_optimization_problem→data_model_to_optimization_problemsolve.hpp/solve.cuhdeclarations, callers, and all test call sites. No deprecation shim (the name is now consistent withdata_model_t).Python bindings for the LP writer
write_lpCython extern andDataModel.writeLP(...)/Problem.writeLP(...), mirroringwriteMPS.Tests
LP_WRITER_TEST— 6 round-trip cases (simple LP, maximize+offset, MIP generals/binaries, free/fixed bounds, quadratic objective, quadratic constraint).test_write_lp_round_tripintest_parser.py(parse LP →writeLP→ read back → assert model equivalence).Issue
Checklist