Skip to content

Initialize the objective sense of mps_data_model_t to minimize - #2181

Merged
rapids-bot[bot] merged 2 commits into
NVIDIA:mainfrom
nicoloangileri:fix/mps-data-model-default-sense
Oct 23, 2026
Merged

rapids-bot[bot] merged 2 commits into
NVIDIA:mainfrom
nicoloangileri:fix/mps-data-model-default-sense

Conversation

@nicoloangileri

@nicoloangileri nicoloangileri commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

mps_data_model_t::maximize_ is the only data member without a default member initializer, and the default constructor is = default, so a model built through the C++ API without an explicit set_maximize() call has an indeterminate objective sense. solve() copies it into the optimization problem (mps_data_model_to_optimization_problem), and mps_writer_t writes OBJSENSE MAXIMIZE when it happens to read as true.

data_model_view_t already defaults to maximize_{false}; this PR does the same for mps_data_model_t. All internal callers (MPS/LP parsers, presolve, pseudo-costs, solve.cu) already call set_maximize(), so their behavior does not change.

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation
    • The documentation is up to date with these changes
    • Added new documentation
    • NA

@nicoloangileri
nicoloangileri requested a review from a team as a code owner October 16, 2026 23:48
@copy-pr-bot

copy-pr-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Source mode: Auto
  • Repository: NVIDIA/cuopt
  • Branch: main
  • File: .coderabbit.yaml
  • Commit: 6cce8f7
  • Layers: Repository YAML
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 98a13030-b6ad-471c-98e0-e6d855c3b6fd
📥 Commits

Reviewing files that changed from the base of the PR and between f7e6646 and ba04f1f.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/cuopt/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Enterprise
  • Run ID: 66ea215c-8ae9-4c51-8d30-5dbb72da4402
📥 Commits

Reviewing files that changed from the base of the PR and between 02bfe53 and 0a36960.

📒 Files selected for processing: 2
  • cpp/include/cuopt/mathematical_optimization/io/mps_data_model.hpp
  • cpp/tests/linear_programming/parser_test.cpp

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

📝 Walkthrough

The MPS data model now initializes its objective sense to minimize. Tests verify this default after construction in nonzeroed storage and after an empty-constraint-matrix round trip.

Changes: MPS default sense

Layer / File(s) Summary
Initialize and verify default sense
cpp/include/cuopt/mathematical_optimization/io/mps_data_model.hpp, cpp/tests/linear_programming/parser_test.cpp
maximize_ now defaults to false. Tests check the default after placement construction in nonzeroed storage and verify that an empty-constraint-matrix round trip retains minimize sense.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mlubin

Merge Risk: ⚪ Minimal · up to 5d799

Merge Risk: ⚪ Minimal · up to 5d799

Default-created MPS models now use minimize, and the round-trip test checks that behavior. No actionable merge risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description clearly explains that mps_data_model_t::maximize_ now defaults to false and describes the affected behavior, existing callers, and testing status.
Title check Passed The title clearly and concisely states the main change: initializing the objective sense of mps_data_model_t to minimize.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

}
}

TEST(mps_data_model, default_sense_is_minimize)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete this test. We don't need such an invasive test to check default values for every struct field.

@mlubin mlubin self-assigned this Oct 23, 2026
@nicoloangileri
nicoloangileri force-pushed the fix/mps-data-model-default-sense branch from 5d7997f to f7e6646 Compare October 23, 2026 13:42
@mlubin mlubin added bug Something isn't working non-breaking Introduces a non-breaking change labels Oct 23, 2026
mps_data_model_t's default constructor left maximize_ uninitialized; it is
the only data member without a default member initializer. A model built
through the public C++ API without an explicit set_maximize() call could
therefore be solved or written to MPS as a maximization problem,
depending on whatever bytes happened to be in memory.

Default the sense to minimize, as data_model_view_t already does, and
add a test that default-constructs the model over non-zero storage.

Signed-off-by: Nicolò Angileri <nicoloangileri635@gmail.com>
As requested in review, the one-line initializer doesn't need a dedicated test.

Signed-off-by: Nicolò Angileri <nicoloangileri635@gmail.com>
@mlubin
mlubin force-pushed the fix/mps-data-model-default-sense branch from f7e6646 to ba04f1f Compare October 23, 2026 14:29
@mlubin

mlubin commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

/ok to test ba04f1f

@mlubin

mlubin commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

/merge

@github-actions

Copy link
Copy Markdown

CI Test Summary

✅ All 21 test job(s) passed.

@rapids-bot
rapids-bot Bot merged commit 9f662f2 into NVIDIA:main Oct 23, 2026
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants