B&B logs deduplication#1352
Conversation
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…an method. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…ogs to use the std::format variant. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
requested review from
aliceb-nv and
chris-maes
and removed request for
kaatish and
yuwenchen95
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors branch-and-bound reporting to centralize table headers and gap-to-percentage formatting, adds C++20 std::format logging helpers to logger_t, and updates deterministic sync and cut-pass logging to use the new formatting and header emission. ChangesLogging and branch-and-bound reporting refactor
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/branch_and_bound/mip_node.hpp (1)
223-227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix printf format for
uint64_tnode_idin B&B debug logging
cpp/src/branch_and_bound/mip_node.hppuses%ldforcurrent_node->node_id(typeuint64_t) intraverse_children(), which is non-portable and can produce undefined behavior.Proposed fix
+#include <cinttypes> ... - printf("Node %ld with no children at depth %d lower bound %e. status %d\n", + printf("Node %" PRIu64 " with no children at depth %d lower bound %e. status %d\n", current_node->node_id, current_node->depth, current_node->lower_bound, current_node->status);🤖 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/branch_and_bound/mip_node.hpp` around lines 223 - 227, The printf in traverse_children uses %ld for current_node->node_id (type uint64_t) which is non-portable; update the format to use the correct uint64_t specifier (e.g., include <inttypes.h> and use "%" PRIu64 with current_node->node_id) so the node_id prints portably, or alternatively cast to (unsigned long long) and use "%llu"; adjust the printf call in traverse_children accordingly.
🤖 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/src/branch_and_bound/search_tree.hpp`:
- Around line 65-70: The issue is that num_nodes (an omp_atomic_t<i_t>) can
overflow when i_t is a 32-bit int while node IDs are stored as uint64_t; update
the counter to use a 64-bit atomic and stop deriving IDs from a narrower type.
Change num_nodes to an atomic type sized to 64 bits (e.g.,
omp_atomic_t<uint64_t> or std::atomic<uint64_t>), ensure branch() calls use
num_nodes.fetch_add(2) returning a uint64_t, and assign the two child IDs
deterministically (use the returned base id and base id+1 rather than
pre-incrementing ++id) in the mip_node_t constructor calls; apply the same
change to the other occurrence around the second mentioned location (the code
near the 156-157 reference).
- Around line 8-12: The header
cuopt::linear_programming::dual_simplex::search_tree.hpp must be made
self-contained: add explicit standard headers for any std symbols it uses (e.g.,
<vector>, <memory>, <optional>, <string>, <mutex>, <cstdint> or others actually
referenced) rather than relying on transitive includes from mip_node.hpp; update
search_tree.hpp to `#include` those standard headers at top and keep the existing
`#include` <branch_and_bound/mip_node.hpp>, so all symbols used in the
declarations inside the namespace are defined by the header itself.
In `@cpp/src/dual_simplex/logger.hpp`:
- Around line 95-96: The code mutates the shared string msg by replacing a
trailing '\n' with '\0' before calling CUOPT_LOG_INFO, which removes the newline
for subsequent file logging; instead, avoid mutating msg in logger.hpp by
creating a temporary view or a separate string (e.g., msg_no_nl) for the
console/formatting path and leave msg intact for file output; update all
occurrences that replace msg.back() (the instances around CUOPT_LOG_INFO and the
corresponding lines mentioned) to use the temporary non-mutated value when
formatting console output so file logging still receives the original msg with
its newline.
- Around line 19-20: logger.hpp uses std::forward (used in the variadic
forwarding calls around where the logging helper templates invoke
std::forward<Args&&>(args)) but doesn't include <utility>, relying on transitive
includes; fix this by adding `#include` <utility> to the top of
cpp/src/dual_simplex/logger.hpp (alongside the existing `#include` <format>) so
the file is self-contained and IWYU-compliant.
---
Outside diff comments:
In `@cpp/src/branch_and_bound/mip_node.hpp`:
- Around line 223-227: The printf in traverse_children uses %ld for
current_node->node_id (type uint64_t) which is non-portable; update the format
to use the correct uint64_t specifier (e.g., include <inttypes.h> and use "%"
PRIu64 with current_node->node_id) so the node_id prints portably, or
alternatively cast to (unsigned long long) and use "%llu"; adjust the printf
call in traverse_children accordingly.
🪄 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: 9a934bf0-2a56-424f-8d93-17eb639ab669
📒 Files selected for processing (5)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/mip_node.hppcpp/src/branch_and_bound/search_tree.hppcpp/src/dual_simplex/logger.hpp
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
|
Let's chat before merging |
Sure |
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
| #include <cstdarg> | ||
| #include <cstdio> | ||
| #include <cstring> | ||
| #include <format> |
There was a problem hiding this comment.
is a very heavy heade in terms of compilation times in most c++ implementations. I'd be in favor of keeping it out of common headers and only use it in the TUs where it is required
There was a problem hiding this comment.
I did not notice a significant increase in compilation time by including or using the std::format. Maybe if it is used everywhere
There was a problem hiding this comment.
BTW, we can skip the compile-time checks of std::format with std::vformat. Although I would say the former is better.
…ly with restarts. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/branch_and_bound/search_tree.hpp (1)
39-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
progresswill stop reflecting deep-tree work accurately.
progress += std::ldexp(f_t(1), -node_ptr->depth)accumulates tree weight in the solver scalar type. Once depth exceeds the mantissa range, contributions are rounded away; for even deeper nodes they underflow to zero. Since this PR turns tree weight into the B&B progress signal, deep subtrees can stop contributing and the reported progress becomes systematically wrong. Store this metric in a dedicated higher-precision or exact representation instead off_t.As per coding guidelines: "Prevent numerical instability (overflow, underflow, precision loss) producing wrong results".
Also applies to: 159-159
🤖 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/branch_and_bound/search_tree.hpp` around lines 39 - 40, The progress accumulation uses the solver scalar f_t and loses contributions for deep node_ptr->depth because of mantissa/underflow; change the representation of progress to a higher-precision or exact type (e.g., long double or a boost::multiprecision binary/decimal type) and perform the std::ldexp and addition in that type instead of f_t so deep-tree weights do not round to zero; update the declaration/initialization of progress and any uses (search_tree::progress, the accumulation sites at the shown ++num_final_nodes / progress += std::ldexp(f_t(1), -node_ptr->depth) and the analogous line at 159) to use the new higher-precision type while converting to f_t only when strictly needed for downstream solver calculations.Source: Coding guidelines
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/search_tree.hpp (1)
27-27: ⚡ Quick winMake this converting constructor
explicit.This currently allows implicit
mip_node_t<i_t, f_t>→search_tree_t<i_t, f_t>conversions.Suggested change
- search_tree_t(mip_node_t<i_t, f_t>&& node) : search_tree_t() { root = std::move(node); } + explicit search_tree_t(mip_node_t<i_t, f_t>&& node) : search_tree_t() + { + root = std::move(node); + }As per coding guidelines: "Use
expliciton single-argument constructors in C++ classes".🤖 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/branch_and_bound/search_tree.hpp` at line 27, The single-argument converting constructor search_tree_t(mip_node_t<i_t, f_t>&& node) should be made explicit to prevent unintended implicit conversions from mip_node_t<i_t, f_t> to search_tree_t<i_t, f_t>; update the constructor signature for search_tree_t to add the explicit keyword (i.e., change the constructor definition for search_tree_t that takes mip_node_t<i_t, f_t>&& to be explicit) while leaving the implementation otherwise unchanged.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.
Outside diff comments:
In `@cpp/src/branch_and_bound/search_tree.hpp`:
- Around line 39-40: The progress accumulation uses the solver scalar f_t and
loses contributions for deep node_ptr->depth because of mantissa/underflow;
change the representation of progress to a higher-precision or exact type (e.g.,
long double or a boost::multiprecision binary/decimal type) and perform the
std::ldexp and addition in that type instead of f_t so deep-tree weights do not
round to zero; update the declaration/initialization of progress and any uses
(search_tree::progress, the accumulation sites at the shown ++num_final_nodes /
progress += std::ldexp(f_t(1), -node_ptr->depth) and the analogous line at 159)
to use the new higher-precision type while converting to f_t only when strictly
needed for downstream solver calculations.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/search_tree.hpp`:
- Line 27: The single-argument converting constructor
search_tree_t(mip_node_t<i_t, f_t>&& node) should be made explicit to prevent
unintended implicit conversions from mip_node_t<i_t, f_t> to search_tree_t<i_t,
f_t>; update the constructor signature for search_tree_t to add the explicit
keyword (i.e., change the constructor definition for search_tree_t that takes
mip_node_t<i_t, f_t>&& to be explicit) while leaving the implementation
otherwise unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 43e2811d-3b3b-4e2b-8167-5c6de6a416e9
📒 Files selected for processing (2)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/search_tree.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/branch_and_bound/branch_and_bound.cpp
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
aliceb-nv
left a comment
aliceb-nv
left a comment
There was a problem hiding this comment.
Thank you! Just a minor nitpick.
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
| void branch_and_bound_t<i_t, f_t>::print_table_header() | ||
| { | ||
| std::string header = | ||
| std::format("{:^{}}|{:^{}}|{:^{}}|{:^{}}|{:^{}}|{:^{}}|{:^{}}|{:^{}}|{:^{}}|", |
There was a problem hiding this comment.
I find this significantly harder to read and understand than the printf format string. It's very easy to read a printf format string and understand the width and precision of each number printed out. I.e. %12.6e means to use a width of 12 with 6 digits of precision after the decimal. The following code is very difficult to read because I now must look up the width from the constants. Also it tells me nothing about the precision after the decimal point.
There was a problem hiding this comment.
I can revert the changes with the constants, so it easier to read.
This specifically is just for the header of the table (so no precision). For the actual formatting for float, it is quite similar actually with the standard C formatting, e.g., {:>12.6f} is right-aligned, width 12 and precision 6. The main advantage here is to avoid manually setting the spacing for each log line and aligning to right/center/left. The syntax is the same that Python.
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
Let's discuss this more. I find the current changes harder to read and understand than the previous code.
|
I will unify all log changes into #1402 |
3 participants
Changes
std::formatto be easier to see the spacing for each column.std::formatsyntax to thelogger_tChecklist