(feature) add distance-based routing ev-breaks#1147
(feature) add distance-based routing ev-breaks#1147np96 wants to merge 10 commits intoNVIDIA:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds EV charging modeled as distance-windowed vehicle breaks across C++ headers/implementation, GPU kernels, special-node handling, and Python/Cython bindings; introduces distance-bounded break records, centralized break-location validation, kernel pruning by distance, and new C++/Python unit tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/routing/ges/squeeze.cuh (1)
464-497:⚠️ Potential issue | 🔴 CriticalGuard the no-candidate path before creating a break node.
If every insertion position is filtered out by this new distance-window check,
thread_best_break_node_idstays-1. Line 490 then passes that invalid index tocreate_break_node(...), which turns an infeasible EV-break route into an out-of-bounds device access instead of a clean “no insertion” outcome.💡 Suggested fix
- if (threadIdx.x == reduction_idx) { + if (threadIdx.x == reduction_idx && thread_best_break_node_id >= 0 && + reduction_buf[0] != std::numeric_limits<double>::max()) { auto break_node = create_break_node<i_t, f_t, REQUEST>( break_nodes, thread_best_break_node_id, solution.problem.dimensions_info); // do not update the intra indices yet sh_route.insert_node(thread_best_idx, break_node, solution.route_node_map, false); route_t<i_t, f_t, REQUEST>::view_t::compute_forward(sh_route);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/routing/ges/squeeze.cuh` around lines 464 - 497, The reduction can yield no valid candidate (thread_best_break_node_id remains -1) so guard the creation/insertion path: after block_reduce_ranked and the reduction-index check, verify thread_best_break_node_id is not -1 (or thread_best_cost is better than the initial sentinel) before calling create_break_node, sh_route.insert_node, compute_forward/backward and sh_route.compute_cost; if no candidate, skip the insertion block entirely (do nothing or set a clear “no-op” state) to avoid passing an invalid index into create_break_node.
🧹 Nitpick comments (4)
python/cuopt/cuopt/tests/routing/test_ev_breaks.py (1)
138-146: Consider using more specific exception type.The test catches a generic
Exception. Consider catching the specific exception type (e.g.,ValueErrororcuopt.CuoptException) to ensure the validation is triggering the expected error path rather than any arbitrary exception.♻️ Proposed refinement
def test_ev_break_invalid_vehicle_id(): """Out-of-range vehicle_id raises an exception.""" d = _small_data_model(n_vehicles=2) - with pytest.raises(Exception): + with pytest.raises(ValueError): # or the specific cuopt exception type d.add_ev_break(5, max_range=100.0, charge_duration=15) - with pytest.raises(Exception): + with pytest.raises(ValueError): d.add_ev_break(-1, max_range=100.0, charge_duration=15)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuopt/cuopt/tests/routing/test_ev_breaks.py` around lines 138 - 146, The test test_ev_break_invalid_vehicle_id currently uses pytest.raises(Exception) which is too broad; update the assertions to expect the specific validation error thrown by d.add_ev_break (e.g., ValueError or cuopt.CuoptException) so the test verifies the correct failure path from the _small_data_model/d.add_ev_break validation, replacing Exception with the concrete exception type your library raises.cpp/tests/routing/unit_tests/ev_breaks.cu (1)
150-159: Multi-cycle assertion may be too strict.The assertion on line 158 checks that every vehicle with any break has exactly 2 breaks. However, if a vehicle has an empty route (no orders assigned), it might legitimately have 0 breaks. The current logic only counts vehicles that appear with at least one break, so this should be fine—but consider documenting this assumption or adding a comment.
Also consider adding edge-case tests for:
- Zero charge duration
- Distance windows that don't overlap with any route distances
- Single-vehicle fleet
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/tests/routing/unit_tests/ev_breaks.cu` around lines 150 - 159, The assertion loop using break_count, h_routing_solution.node_types and h_routing_solution.truck_id that verifies node_type_t::BREAK occurrences with ASSERT_EQ(cnt, 2) assumes vehicles that never appear have 0 breaks and therefore are excluded; add a short comment above the break_count construction documenting this assumption and why vehicles with empty routes are intentionally not asserted, and optionally add new unit tests covering edge cases (zero charge duration, non-overlapping distance windows, single-vehicle fleet) to validate behavior for those scenarios.cpp/src/routing/data_model_view.cu (1)
182-220: Missingvehicle_idvalidation.The existing
add_vehicle_breakmethod (lines 150-180) also lacks explicit vehicle_id bounds checking, butadd_vehicle_ev_breakshould validate thatvehicle_idis within[0, fleet_size_)to provide clear error messages rather than undefined behavior when accessingvehicle_breaks_[vehicle_id].Additionally, the break-location validation logic (lines 198-212) duplicates the pattern in
add_vehicle_break(lines 164-178). Consider extracting a helper function.🛡️ Proposed fix to add vehicle_id validation
template <typename i_t, typename f_t> void data_model_view_t<i_t, f_t>::add_vehicle_ev_break(i_t vehicle_id, f_t distance_min, f_t distance_max, i_t charge_duration, i_t const* break_locations, i_t num_break_locations, bool validate_input) { + cuopt_expects(vehicle_id >= 0 && vehicle_id < fleet_size_, + error_type_t::ValidationError, + "vehicle_id must be in [0, fleet_size)"); cuopt_expects(distance_max > distance_min, error_type_t::ValidationError, "EV break distance_max must be greater than distance_min!");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/routing/data_model_view.cu` around lines 182 - 220, add_vehicle_ev_break is missing bounds checking for vehicle_id and duplicates break-location validation logic also repeats code from add_vehicle_break; add a cuopt_expects check at the top of data_model_view_t::add_vehicle_ev_break to ensure vehicle_id is in [0, fleet_size_) and emits a ValidationError with a clear message before accessing vehicle_breaks_[vehicle_id], and refactor the duplicated break-location validation block (the validate_input && num_break_locations > 0 logic that uses detail::check_min_max_values, rmm::device_uvector, raft::copy, thrust::unique and cuopt_expects) into a reusable helper (e.g. detail::validate_and_check_unique_break_locations or a private method) so both add_vehicle_break and add_vehicle_ev_break call that helper instead of duplicating the code.cpp/src/routing/problem/special_nodes.cuh (1)
58-61: Inconsistent emptiness check methods.Line 58 uses
distance_min.empty()(raft::device_span method) while line 96 usesdistance_min.is_empty()(rmm::device_uvector method). While both work correctly in their respective contexts (span vs uvector), consider using consistent naming patterns for clarity.Also applies to: 96-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/routing/problem/special_nodes.cuh` around lines 58 - 61, The code mixes emptiness checks (.empty() from raft::device_span and .is_empty() from rmm::device_uvector); make them consistent by calling .empty() only on raft::device_span instances (e.g., when checking spans before assigning v.distance_min/v.distance_max) and .is_empty() on rmm::device_uvector instances (the checks around the distance_min/distance_max uvector usage at lines ~96-99), updating whichever checks are currently using the wrong method so each type uses its correct emptiness accessor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuopt/cuopt/routing/vehicle_routing.py`:
- Around line 395-452: The add_ev_break method currently allows invalid inputs
(n_cycles <= 0, negative/zero max_range, min_range out of bounds, and
unvalidated charging_stations) which can silently produce incorrect behavior;
update add_ev_break to (1) validate n_cycles: if n_cycles is None set to 1 else
ensure it is an int >= 1 and raise ValueError for 0 or negative, (2) validate
max_range is a positive number and min_range is >= 0 and <= max_range (raise
ValueError otherwise), and (3) validate charging_stations is the expected
type/contents (e.g., cudf.Series of ints or None) before looping; keep using
validate_range for vehicle id checks and call super().add_vehicle_ev_break only
after these validations, raising clear ValueError messages referencing the
offending parameter.
---
Outside diff comments:
In `@cpp/src/routing/ges/squeeze.cuh`:
- Around line 464-497: The reduction can yield no valid candidate
(thread_best_break_node_id remains -1) so guard the creation/insertion path:
after block_reduce_ranked and the reduction-index check, verify
thread_best_break_node_id is not -1 (or thread_best_cost is better than the
initial sentinel) before calling create_break_node, sh_route.insert_node,
compute_forward/backward and sh_route.compute_cost; if no candidate, skip the
insertion block entirely (do nothing or set a clear “no-op” state) to avoid
passing an invalid index into create_break_node.
---
Nitpick comments:
In `@cpp/src/routing/data_model_view.cu`:
- Around line 182-220: add_vehicle_ev_break is missing bounds checking for
vehicle_id and duplicates break-location validation logic also repeats code from
add_vehicle_break; add a cuopt_expects check at the top of
data_model_view_t::add_vehicle_ev_break to ensure vehicle_id is in [0,
fleet_size_) and emits a ValidationError with a clear message before accessing
vehicle_breaks_[vehicle_id], and refactor the duplicated break-location
validation block (the validate_input && num_break_locations > 0 logic that uses
detail::check_min_max_values, rmm::device_uvector, raft::copy, thrust::unique
and cuopt_expects) into a reusable helper (e.g.
detail::validate_and_check_unique_break_locations or a private method) so both
add_vehicle_break and add_vehicle_ev_break call that helper instead of
duplicating the code.
In `@cpp/src/routing/problem/special_nodes.cuh`:
- Around line 58-61: The code mixes emptiness checks (.empty() from
raft::device_span and .is_empty() from rmm::device_uvector); make them
consistent by calling .empty() only on raft::device_span instances (e.g., when
checking spans before assigning v.distance_min/v.distance_max) and .is_empty()
on rmm::device_uvector instances (the checks around the
distance_min/distance_max uvector usage at lines ~96-99), updating whichever
checks are currently using the wrong method so each type uses its correct
emptiness accessor.
In `@cpp/tests/routing/unit_tests/ev_breaks.cu`:
- Around line 150-159: The assertion loop using break_count,
h_routing_solution.node_types and h_routing_solution.truck_id that verifies
node_type_t::BREAK occurrences with ASSERT_EQ(cnt, 2) assumes vehicles that
never appear have 0 breaks and therefore are excluded; add a short comment above
the break_count construction documenting this assumption and why vehicles with
empty routes are intentionally not asserted, and optionally add new unit tests
covering edge cases (zero charge duration, non-overlapping distance windows,
single-vehicle fleet) to validate behavior for those scenarios.
In `@python/cuopt/cuopt/tests/routing/test_ev_breaks.py`:
- Around line 138-146: The test test_ev_break_invalid_vehicle_id currently uses
pytest.raises(Exception) which is too broad; update the assertions to expect the
specific validation error thrown by d.add_ev_break (e.g., ValueError or
cuopt.CuoptException) so the test verifies the correct failure path from the
_small_data_model/d.add_ev_break validation, replacing Exception with the
concrete exception type your library raises.
🪄 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: e3ba6d4d-7b0c-469f-90a7-dd7bd75329f3
📒 Files selected for processing (13)
cpp/include/cuopt/routing/data_model_view.hppcpp/include/cuopt/routing/routing_structures.hppcpp/src/routing/data_model_view.cucpp/src/routing/ges/squeeze.cuhcpp/src/routing/local_search/breaks_insertion.cucpp/src/routing/problem/problem.cucpp/src/routing/problem/special_nodes.cuhcpp/tests/routing/CMakeLists.txtcpp/tests/routing/unit_tests/ev_breaks.cupython/cuopt/cuopt/routing/vehicle_routing.pxdpython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/routing/vehicle_routing_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_ev_breaks.py
| self, vehicle_id, distance_min, distance_max, duration, locations | ||
| ): | ||
| dim = 0 | ||
| if vehicle_id in self.non_uniform_breaks: |
There was a problem hiding this comment.
you can replace the whole segment
dim = 0
if vehicle_id in self.non_uniform_breaks:
dim = len(self.non_uniform_breaks[vehicle_id])
if dim == 0:
self.non_uniform_breaks[vehicle_id] = {}
with this
dim = self.non_uniform_breaks.setdefault(vehicle_id, {})
|
BTW, regarding this:
It is about C++ source code, right? Not the tests? |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/routing/ges/squeeze.cuh`:
- Around line 489-490: The if-condition contains a duplicated logical operator;
edit the condition in the block using threadIdx.x, reduction_idx,
thread_best_break_node_id, and reduction_buf so it reads: if (threadIdx.x ==
reduction_idx && thread_best_break_node_id >= 0 && reduction_buf[0] !=
std::numeric_limits<double>::max()) — i.e., remove the extra && token between
reduction_idx and thread_best_break_node_id so the expression compiles
correctly.
In `@python/cuopt/cuopt/routing/vehicle_routing.py`:
- Around line 405-440: The add_ev_break public method lacks Returns and Raises
sections in its docstring; update the docstring for add_ev_break in
vehicle_routing.py to include a Returns section (e.g., return type and brief
description such as None or the updated routing object/side-effect) and a Raises
section documenting the exceptions callers can expect (for example TypeError for
invalid vehicle_ids or charging_stations types, ValueError for invalid numeric
parameters like negative max_range/min_range or n_cycles, and any other
exceptions raised by internal validation). Keep the text concise, reference the
key parameters (vehicle_ids, max_range, min_range, n_cycles, charging_stations)
and match the existing pydocstyle formatting.
- Around line 396-404: The public function add_ev_break lacks type hints; update
its signature to include parameter and return type annotations: annotate
vehicle_ids (e.g., Sequence[int] or cudf.Series depending on usage), max_range
and min_range as float, charge_duration as float or int, charging_stations as
cudf.Series (or Optional[cudf.Series]), n_cycles as Optional[int], and the
return type (e.g., None or the actual return type used by add_ev_break). Ensure
imports for typing (Optional, Sequence) are added if needed and keep the
function name add_ev_break and its parameter names unchanged.
In `@python/cuopt/cuopt/tests/routing/test_ev_breaks.py`:
- Around line 192-197: The test test_invalid_n_cycles_wrong_type incorrectly
expects a ValueError for n_cycles=None but the API accepts None (defaults to one
cycle); update the test by removing None from the parametrized values (leave
only invalid types like 1.5 and "3") or split into two tests: one that asserts
n_cycles=None is accepted (calling model.add_ev_break with n_cycles=None and
asserting no exception or expected state) and another that asserts non-integer
types (1.5, "3") raise ValueError when calling model.add_ev_break.
🪄 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: e5fc96fa-9f5d-4080-8ffd-06f8e745b9f1
📒 Files selected for processing (7)
cpp/src/routing/data_model_view.cucpp/src/routing/ges/squeeze.cuhcpp/src/routing/local_search/breaks_insertion.cucpp/tests/routing/unit_tests/ev_breaks.cupython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/routing/vehicle_routing_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_ev_breaks.py
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/tests/routing/unit_tests/ev_breaks.cu
- python/cuopt/cuopt/routing/vehicle_routing_wrapper.pyx
- cpp/src/routing/data_model_view.cu
np96
force-pushed
the
feature/ev-distance-breaks
branch
from
2e7ff3a to
6581782
Compare
np96
force-pushed
the
feature/ev-distance-breaks
branch
from
6581782 to
c98ca8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuopt/cuopt/tests/routing/test_ev_breaks.py`:
- Line 9: Remove the unused import by deleting the line that imports utils (the
"from cuopt.routing import utils" statement) from the test module; ensure no
other code references the symbol `utils` in functions/classes within this file
(e.g., test cases in test_ev_breaks.py) before removing to avoid breaking
imports.
🪄 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: 66456cbe-e593-41f1-a16a-65297a8fb91a
📒 Files selected for processing (7)
cpp/src/routing/data_model_view.cucpp/src/routing/ges/squeeze.cuhcpp/src/routing/local_search/breaks_insertion.cucpp/tests/routing/unit_tests/ev_breaks.cupython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/routing/vehicle_routing_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_ev_breaks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/routing/data_model_view.cu
np96
force-pushed
the
feature/ev-distance-breaks
branch
from
c98ca8f to
f61e60f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/src/routing/data_model_view.cu (2)
211-220: Consider validating thatdistance_minis non-negative.The method validates
distance_max > distance_minandcharge_duration >= 0, but does not check thatdistance_min >= 0. A negative distance minimum is semantically invalid for a distance-based charging window.🛡️ Proposed fix
cuopt_expects(0 <= vehicle_id && vehicle_id < fleet_size_, error_type_t::ValidationError, "vehicle_id must be in [0, fleet_size)"); + cuopt_expects(distance_min >= 0, + error_type_t::ValidationError, + "distance_min must be non-negative!"); cuopt_expects(distance_max > distance_min, error_type_t::ValidationError, "EV break distance_max must be greater than distance_min!");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/routing/data_model_view.cu` around lines 211 - 220, Add a validation that distance_min is non-negative: in the same block where cuopt_expects is used (near the checks for vehicle_id, distance_max > distance_min, and charge_duration >= 0), add a cuopt_expects(distance_min >= 0, error_type_t::ValidationError, "distance_min must be non-negative!") so the function validates distance_min along with distance_max and charge_duration; keep the same error type and style as the existing cuopt_expects calls.
28-32: Misleading error message for bounds check.The error message states "Break locations should be at the end of the matrix" but the check validates that locations are within
[0, num_locations - 1]. This message is inconsistent withset_break_locationsat Line 112 which uses the same text for the same check.Consider clarifying:
- "Break locations should be at the end of the matrix"); + "Break locations must be within [0, num_locations - 1]");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/routing/data_model_view.cu` around lines 28 - 32, The current error message used with cuopt::cuopt_expects around cuopt::routing::detail::check_min_max_values is misleading — it asserts "Break locations should be at the end of the matrix" while the check actually validates bounds [0, num_locations - 1]; update the message used in this call (and in set_break_locations) to clearly state the bounds validation (e.g., "Break locations must be within [0, num_locations - 1]" or similar) so the text matches the check performed by cuopt::routing::detail::check_min_max_values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/src/routing/data_model_view.cu`:
- Around line 211-220: Add a validation that distance_min is non-negative: in
the same block where cuopt_expects is used (near the checks for vehicle_id,
distance_max > distance_min, and charge_duration >= 0), add a
cuopt_expects(distance_min >= 0, error_type_t::ValidationError, "distance_min
must be non-negative!") so the function validates distance_min along with
distance_max and charge_duration; keep the same error type and style as the
existing cuopt_expects calls.
- Around line 28-32: The current error message used with cuopt::cuopt_expects
around cuopt::routing::detail::check_min_max_values is misleading — it asserts
"Break locations should be at the end of the matrix" while the check actually
validates bounds [0, num_locations - 1]; update the message used in this call
(and in set_break_locations) to clearly state the bounds validation (e.g.,
"Break locations must be within [0, num_locations - 1]" or similar) so the text
matches the check performed by cuopt::routing::detail::check_min_max_values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b5d2392-bb35-4193-aea8-f7037822e2e1
📒 Files selected for processing (7)
cpp/src/routing/data_model_view.cucpp/src/routing/ges/squeeze.cuhcpp/src/routing/local_search/breaks_insertion.cucpp/tests/routing/unit_tests/ev_breaks.cupython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/routing/vehicle_routing_wrapper.pyxpython/cuopt/cuopt/tests/routing/test_ev_breaks.py
✅ Files skipped from review due to trivial changes (1)
- cpp/src/routing/local_search/breaks_insertion.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/routing/ges/squeeze.cuh
- python/cuopt/cuopt/tests/routing/test_ev_breaks.py
np96
force-pushed
the
feature/ev-distance-breaks
branch
from
f61e60f to
1ddca47
Compare
rg20
added
feature request
Signed-off-by: np96 <n.poperechnyi@gmail.com>
|
Hi @rgsl888prabhu |
Thank you @np96, let me review. |
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Thank you @np96 for the PR, I have a minor suggestion, it would be really useful if we can add an example under doc for python.
|
/ok to test d4bd1f8 |
Thanks @rgsl888prabhu for reviewing @np96 's impactful PR ! |
Great idea. That was very helpful, After adding I found out that the solver may intentionally generate infeasible solutions and small fix in breaks_insertion.cu was needed. Besides this, added a unit test which reproduces the issue and aligned python/cython naming. There is still server side support not implemented. If I have time, I'll do this in 26.06 release, otherwise in 26.08. Thank you. @anandhkb FYI. |
|
/ok to test bfdcead |
…st, fix method naming Signed-off-by: np96 <n.poperechnyi@gmail.com>
np96
force-pushed
the
feature/ev-distance-breaks
branch
from
bfdcead to
df27483
Compare
|
@rgsl888prabhu for some reason pre-commit hook wasn't activated on signoff. |
|
Yeah, there is a ongoing issue in wheel build due to scache bug |
Thanks @np96 !, makes perfect sense! |
|
Thanks for adding the distance based breaks to cuOpt. I have a few comments on the implementation. Do you see the distance_min and max as hard-constraints or soft-constraints (i.e. objective to be minimized)? I see that you are directly encoding the infeasibilities and violation as part of break insertion kernel. This is not the correct way to do it. The infeasibility calculations should be consistent across all the operators. Even when the route is infeasible because distance constraints are violated, other operators see the route as feasible and move on. If the infeasibilities are handled correctly, the other operators have the opportunity to kick off some other nodes (non-break nodes) to make the route feasible. We handle all the infeasibilities and objectives within the route and node data structures. A detailed logic of how to setup forward and backward calculations to be able to compute infeasibilities and objectives can be found here: https://github.com/NVIDIA/cuopt/blob/scheduling/skills/cuopt-developer/vrp_skills.md . The correct way to do this is by moving the infeasibility metric (or objective) to distance dimension. This needs adding few more quantities to the distance_node_t and distance_route_t. From API point of view, I would advise to use distance_based_break instead of ev_based to make it more general. This feature will be greatly useful to impose driver rules (for example, driver can drive for more than 100 miles without a break). |
|
Hi @rg20.
I believe these should be hard-constraints, though I am not quite sure with the semantics. The ones proposed currently may not be ideal, but they look simplest to implement. Do you agree with the API? I'd be glad to hear a better idea. Thanks. |
|
/ok to test df27483 |
@np96 I agree with the API, except for the naming. The current API makes it look like it is very specific to EVs. In fact, this is a much more general use case. Often, there are regulations on how far a driver can drive continuously without a break. This feature makes it possible to model such scenarios. Glad you agree with the approach. Looking forward for the changes. Can you also provide a dataset with reasonable sizes so that we can use it for regression testing? |
|
@rg20 regarding dataset. You want all-at-once or solely dist breaks? |
Yes, what I meant earlier was to keep on integration test which hits all the api parts and small. So we are sure everything works and doesn't take too much time. |
|
/ok to test bfe3e8a |
Typically we want some unit tests that test this feature, but we also like to have a representative use case (that includes this feature) that is used in production. We use such datasets for performance regression testing. |
|
FYI, I shared developer skills file for VRP with you. I used these skills with Claude code to develop a new feature for another customer. May be you can give it a try. You will have to describe the penalty function and ask it to develop. |
4 participants
Description
Adds
add_vehicle_ev_break(vehicle_id, distance_min, distance_max, charge_duration, locations)to the C++ and Python APIs;Extends
vehicle_break_twith anis_distance_basedflag anddistance_min/distance_maxfields; the existing break call infrastructure handles placement;The solver inserts one charging stop per call within
[distance_min, distance_max]of cumulative route distance; call the function multiple times to model successive charge cycles (e.g.[0, 150], [150, 300]);No new solver dimension — the distance window is checked against distance_forward during forward propagation, reusing the same feasibility path as time-windowed breaks;
Charging stations are specified per-vehicle and per-cycle via break_locations.
Issue
#589
Checklist
I am familiar with the Contributing Guidelines.