Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions cpp/src/grpc/codegen/FIELD_REGISTRY_REFERENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,10 @@ section:
(the setter / assignment is skipped). This is the case the flag is designed
for: without `optional:`, an external client that omits the field silently
overwrites the C++ default with the proto3 zero.
- **Problem-chunked**: currently *not honored* — the chunked path reads from
the hand-written `ChunkedProblemHeader` message which does not declare
`optional` on its fields. Tracked for unification (see §6).
- **Problem-chunked**: the C++ field's in-class default is preserved, provided
the matching field in the hand-written `ChunkedProblemHeader` message is
also declared `optional`. The generator emits a `has_X()` guard for registry
fields marked `optional`.
- **Solution**: cosmetic. Solutions are constructor-built each call; there's no
pre-existing default to preserve. The `optional` keyword adds `has_X()` to
the proto for client-side presence detection, but the from-proto path still
Expand Down
4 changes: 4 additions & 0 deletions cpp/src/grpc/codegen/field_registry.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,10 @@ optimization_problem:
getter: get_sense()
- objective_scaling_factor:
field_num: 4
# C++ defaults to 1 while an absent proto3 scalar reads as 0. Track
# presence so external clients may omit the field without changing the
# objective to an unsupported zero scaling factor.
optional: true
- objective_offset:
field_num: 5

Expand Down
10 changes: 5 additions & 5 deletions cpp/src/grpc/codegen/generate_conversions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2420,22 +2420,22 @@ def _gen_chunked_header_to_problem(registry, indent=" "):
ind = indent
lines = []

# `ChunkedProblemHeader` is hand-written (see cuopt_remote_service.proto);
# it does not declare `optional` for any field, so we pass has_check=None
# to suppress the has_X() guard. `sentinel:` still applies — the wire
# mapping must stay consistent with the unary path.
# `ChunkedProblemHeader` is hand-written (see cuopt_remote_service.proto).
# Fields marked optional in the registry must also be declared optional in
# that message so the chunked path can preserve non-zero C++ defaults.
for entry in obj.get("scalars", []):
f = parse_field(entry)
pname = _proto_cpp_name(f["name"])
setter = _default_problem_setter(f)
has_check = f"header.has_{pname}()" if f.get("optional") else None
lines.extend(
emit_scalar_from_proto_assign(
lambda v, s=setter: f"cpu_problem.{s}({v});",
f"header.{pname}()",
f,
registry,
ind,
has_check=None,
has_check=has_check,
)
)

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/grpc/codegen/generated/cuopt_remote_data.proto
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ message OptimizationProblem {
string problem_name = 1;
string objective_name = 2;
bool maximize = 3;
double objective_scaling_factor = 4;
optional double objective_scaling_factor = 4;
double objective_offset = 5;
repeated string variable_names = 7;
repeated string row_names = 8;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
cpu_problem.set_problem_name(header.problem_name());
cpu_problem.set_objective_name(header.objective_name());
cpu_problem.set_maximize(header.maximize());
cpu_problem.set_objective_scaling_factor(header.objective_scaling_factor());
if (header.has_objective_scaling_factor()) {
cpu_problem.set_objective_scaling_factor(header.objective_scaling_factor());
}
cpu_problem.set_objective_offset(header.objective_offset());

if (header.variable_names_size() > 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
cpu_problem.set_problem_name(pb_problem.problem_name());
cpu_problem.set_objective_name(pb_problem.objective_name());
cpu_problem.set_maximize(pb_problem.maximize());
cpu_problem.set_objective_scaling_factor(pb_problem.objective_scaling_factor());
if (pb_problem.has_objective_scaling_factor()) {
cpu_problem.set_objective_scaling_factor(pb_problem.objective_scaling_factor());
}
cpu_problem.set_objective_offset(pb_problem.objective_offset());

if (pb_problem.a_offsets_size() > 0) {
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/grpc/cuopt_remote_service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ message ChunkedProblemHeader {

// Problem scalars
bool maximize = 2;
double objective_scaling_factor = 3;
optional double objective_scaling_factor = 3;
double objective_offset = 4;
string problem_name = 5;
string objective_name = 6;
Expand Down
47 changes: 47 additions & 0 deletions cpp/tests/linear_programming/grpc/grpc_client_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1857,6 +1857,53 @@ TEST(MapperRoundtrip, ProblemWithVariableTypes)
EXPECT_DOUBLE_EQ(restored_obj[2], 3.0);
}

TEST(MapperRoundtrip, UnaryProblemObjectiveScalingPresence)
{
cuopt::remote::OptimizationProblem omitted;
omitted.add_c(0.0);
omitted.add_variable_lower_bounds(0.0);
omitted.add_variable_upper_bounds(1.0);
ASSERT_FALSE(omitted.has_objective_scaling_factor());

cpu_optimization_problem_t<int32_t, double> restored_default;
map_proto_to_problem(omitted, restored_default);
EXPECT_DOUBLE_EQ(restored_default.get_objective_scaling_factor(), 1.0);

cpu_optimization_problem_t<int32_t, double> orig;
const std::vector<double> objective{0.0};
const std::vector<double> lower_bound{0.0};
const std::vector<double> upper_bound{1.0};
orig.set_objective_coefficients(objective.data(), objective.size());
orig.set_variable_lower_bounds(lower_bound.data(), lower_bound.size());
orig.set_variable_upper_bounds(upper_bound.data(), upper_bound.size());
orig.set_objective_scaling_factor(2.5);
cuopt::remote::OptimizationProblem present;
map_problem_to_proto(orig, &present);
ASSERT_TRUE(present.has_objective_scaling_factor());
EXPECT_DOUBLE_EQ(present.objective_scaling_factor(), 2.5);

cpu_optimization_problem_t<int32_t, double> restored_present;
map_proto_to_problem(present, restored_present);
EXPECT_DOUBLE_EQ(restored_present.get_objective_scaling_factor(), 2.5);
}

TEST(MapperRoundtrip, ChunkedProblemObjectiveScalingPresence)
{
cuopt::remote::ChunkedProblemHeader omitted;
ASSERT_FALSE(omitted.has_objective_scaling_factor());

cpu_optimization_problem_t<int32_t, double> restored_default;
map_chunked_header_to_problem(omitted, restored_default);
EXPECT_DOUBLE_EQ(restored_default.get_objective_scaling_factor(), 1.0);

omitted.set_objective_scaling_factor(2.5);
ASSERT_TRUE(omitted.has_objective_scaling_factor());

cpu_optimization_problem_t<int32_t, double> restored_present;
map_chunked_header_to_problem(omitted, restored_present);
EXPECT_DOUBLE_EQ(restored_present.get_objective_scaling_factor(), 2.5);
}
Comment on lines +1860 to +1905

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add regression coverage for an explicitly set zero.

These tests cover omission and 2.5, but not the required boundary case where objective_scaling_factor is explicitly set to 0; add unary and chunked assertions that this present value follows the existing invalid-value path rather than being treated as absent and defaulting to 1.0.

As per path instructions, mapper tests should cover negative cases and the specific bug-fix regression. The PR objective requires explicit 0 to remain invalid.

🤖 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/grpc/grpc_client_test.cpp` around lines 1860 -
1905, Extend UnaryProblemObjectiveScalingPresence and
ChunkedProblemObjectiveScalingPresence with explicit-zero cases: set
objective_scaling_factor to 0, verify the field is present, and assert mapping
follows the existing invalid-value behavior rather than defaulting to 1.0. Keep
the existing omission and 2.5 presence coverage unchanged.

Source: Path instructions


TEST(MapperRoundtrip, MIPSolutionAllFields)
{
std::vector<double> sol_vec = {1.0, 0.0, 1.0, 0.0, 1.0};
Expand Down
Loading