Fix issue #4133: Correct negative transit handling in FinalizeAllowedVehicles#4868
Open
cyril23 wants to merge 3 commits intogoogle:mainfrom
Open
Fix issue #4133: Correct negative transit handling in FinalizeAllowedVehicles#4868cyril23 wants to merge 3 commits intogoogle:mainfrom
cyril23 wants to merge 3 commits intogoogle:mainfrom
Conversation
…ehicles with negative transits The FinalizeAllowedVehicles() function was using abs() on transit values to check against vehicle capacity, which incorrectly excluded vehicles when: - CARGO_LOAD dimensions use negative constants for cumulative load tracking - Reload dimensions use negative values at landfills/depots This fix modifies the capacity check to only apply to positive transit values, since negative transits are used for algorithmic purposes (reload semantics, load tracking) rather than actual cargo capacity requirements. Changes: - Lines 1414-1421: Pre-processing loop now only considers positive transits for max_node_transit calculation - Lines 1444-1448: Main capacity check now skips negative transits entirely This resolves the regression introduced in commit 847cfc9 (Dec 1, 2023) affecting OR-Tools versions 9.9.3963 through 9.14.6206. Fixes: google#4133 Testing: test_ortools_version_standalone_codex.py (multi-class VRP with load tracking)
…llowedVehicles The FinalizeAllowedVehicles() optimization incorrectly excluded vehicles from nodes with negative unary transits. The bug checked abs(transit) against capacity alone, but negative transits are feasible when abs(transit) <= capacity + slack_max (vehicle can absorb the negative transit via both capacity headroom and dimension slack). This caused heterogeneous VRP problems with reload/load-tracking dimensions to incorrectly exclude smaller vehicles, forcing suboptimal solutions with dropped stops. The fix properly bounds negative transits by combining capacity and slack_max, while positive transits continue to check capacity alone. Fixes google#4133
cyril23
added a commit
to LogicsSoftwareGmbH/or-tools
that referenced
this pull request
Oct 13, 2025
…n FinalizeAllowedVehicles (v9.12) Backports the fix from PR google#4868 to v9.12 branch. The FinalizeAllowedVehicles() optimization incorrectly excluded vehicles from nodes with negative unary transits. The bug checked abs(transit) against capacity alone, but negative transits are feasible when abs(transit) <= capacity + slack_max (vehicle can absorb the negative transit via both capacity headroom and dimension slack). This caused heterogeneous VRP problems with reload/load-tracking dimensions to incorrectly exclude smaller vehicles, forcing suboptimal solutions with dropped stops. The fix properly bounds negative transits by combining capacity and slack_max, while positive transits continue to check capacity alone. Changes: - Pre-processing now tracks max positive transit, min (negative) transit, and slack_max separately instead of using abs() on all transits - Capacity optimization check considers both positive and negative bounds - Per-node feasibility check uses proper bounds: transit <= capacity && transit >= min_allowed_transit Tested with regression script from issue google#4133. Fixes google#4133 Backport-of: google#4868
Collaborator
|
Thanks for your explanation and your code, I agree with the general modification, this was an actual bug. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a regression introduced in v9.9 (commit 847cfc9) where
FinalizeAllowedVehicles()incorrectly excluded vehicles from nodes with negative unary transits, causing heterogeneous VRP problems with reload/load-tracking dimensions to produce suboptimal solutions with excessive dropped stops.The Bug
The optimization in
FinalizeAllowedVehicles()checked:This is incorrect for negative transits. A vehicle with
capacity=50000was excluded from nodes withtransit=-70000, even whenslack_max=140000was sufficient to absorb the negative transit.Why PR #4853 Was Incorrect
The previous fix attempted to skip all negative transits:
As @lperron correctly noted, this breaks feasibility guarantees. A negative transit with
abs(transit) > slack_maxshould genuinely exclude vehicles, but that fix allowed them through.The Correct Fix
Negative transits are feasible when the vehicle can absorb them via combined capacity and slack:
This translates to:
transit ≤ capacity(must fit in vehicle)abs(transit) ≤ capacity + slack_max(absorbed via capacity headroom + slack)This is mathematically correct: a vehicle starting with
cumul = capacitycan decrease by up tocapacity + slack_maxbefore violating the lower bound (0).Regression Test
A regression test reproducing the exact bug is available: test_issue_4133_regression.py
Results with this fix:
Expected results on v9.9-9.14 (broken versions):
Impact
Fixes routing problems affected for 20+ months (v9.9.3963 through v9.14.6206):
Fixes #4133