8000 feat(drt): explicit max ride constraints in drt by nkuehnel · Pull Request #3922 · matsim-org/matsim-libs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(drt): explicit max ride constraints in drt #3922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nkuehnel
Copy link
Member
@nkuehnel nkuehnel commented Apr 29, 2025

Explicitly consider max ride duration constraints in the slack time calculation.
Previously, max ride duration was mainly accounted for in the insertion of new trips only, but not for existing trips. The workaround was to also set the maximum pickup delay, such that accepted rides pickups were not changing too much after insertion anymore.

To account for max ride durations explicitly, accepted requests now have additional fields for current (expected) pickup and dropoff times. These need to be updated, which is why I extracted the schedule timing updater interface that also updates these times for DRT...

@sebhoerl could you please also have a quick look, as I saw that sometimes for more complicated stop durations the max ride duration is exceeded by roughly the stop duration which might not be accounted for correctly in the updating of pickup and dropoff times (and, hence, slack times in the vehicle data entry).

Leaving this as a draft for now, feedback welcome

@nkuehnel nkuehnel marked this pull request as draft April 29, 2025 14:52
@nkuehnel nkuehnel marked this pull request as ready for review May 6, 2025 07:54
@sebhoerl
Copy link
Contributor
sebhoerl commented May 7, 2025

Hi Nico, have a look at this issue, I think this is very much related: #3757

I didn't check in detail yet what you did there, but making a difference between "vehicle arrival time" and "passenger dropoff time" and then "vehicle departure time" and "passenger pickup time" is indeed important and needs to be passed through from the InsertionGenerator all the way to the InsertionCostCalculator to handle variable stop durations properly.

@sebhoerl
Copy link
Contributor
sebhoerl commented May 7, 2025

Regarding this PR, if I understand correctly, the point is that the slack does not take into account the ride duration (and max ride duration) if the existing requests yet? So we may find insertions that violate the max ride constraint of existing requests. Now this PR limits the slack such that it is reduced to the amount of time that is left in terms of ride time for the onboard requests without violating the constraints?

So this is only a performance improvement to filter out early irrelevant insertions. Because right now these cases are later thrown out by the InsertionCostCalculator? Or this was not handled at all?

I think the PR looks good, at the end not sure if this is fully related, but the pickup/dropoff fields will be quite useful for resolving #3757 I think.

@nkuehnel
Copy link
Member Author
nkuehnel commented May 7, 2025

@sebhoerl correct it's about ensuring that existing requests' max ride duration is not exceeded. This was not possible so far in the cost calculation, it only worked if you limited the pickup delay as the pickup time window then became narrow enough to ensure that accepted requests do not violate max ride too heavily.

I'll have a look at your linked issue

Copy link
Collaborator
@luchengqi7 luchengqi7 left a comment

Choose a reason for hiding this comment

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

Thanks, Nico, for the nice update! It looks good, and I am looking forward to using the new constraint in my case studies!

One side comments: do you know how the computational speed is impacted? Hopefully there is no major impact :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this LATE_DIVERSION_VIOLATION_PENALTY also correspond to the latest arrival time?

A note based on a chat with Michal several years ago: these values are chosen as the initial values. And it may not be the ideal one, but it works for now.

Copy link
Member Author
@nkuehnel nkuehnel May 7, 2025

Choose a reason for hiding this comment

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

@luchengqi7
The latest arrival time still has its own penalty, these are all the penalties we have now:

MAX_WAIT_TIME_VIOLATION_PENALTY = 1;// 1 second of penalty per 1 second of late departure
MAX_TRAVEL_TIME_VIOLATION_PENALTY = 10;// 10 seconds of penalty per 1 second of late arrival
MAX_RIDE_TIME_VIOLATION_PENALTY = 10;// 10 seconds of penalty per 1 second of exceeded detour
LATE_DIVERSION_VIOLATION_PENALTY = 10;// 1 second of penalty per 1 second of late diversion of onboard requests
LATE_DIVERSION_VIOLATION_PENALTY = 10;// 10 second of penalty per 1 second of late diversion of onboard requests

The late diversion can be configured so that x seconds before the expected dropoff, passengers may not be detoured anymore, even if it would fit into the initial time window contract. However, this was introduced in an earlier PR and should not be affected by this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah and I havent had the chance to really test performance impacts. I can do that before merging

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some informal checking on performance and at least in one scenario it was progressing noticeably faster, but this was likely due to increased rejections. A more correct assessment of performance would be to verify it in an test case where rejections are exactly the same before/after this change..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0