-
Notifications
You must be signed in to change notification settings - Fork 451
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
feat: generalize multimodal link chooser #3536
Conversation
Do we need the other |
@Janekdererste This is just in the default implementation, because this one doesn't care about access/egress, but only about the facility location. So decideAccessLink and decideEgressLink delegate to decideOnLink, otherwise we duplicate the same code. But yes, we can make this one |
Sorry, @sebhoerl, I didn't read carefully enough. I thought the method is still in the interface. You implemented it just like I would have expected :-) |
I really like the idea of making this configurable. However, when looking at the code I noticed one thing that might be problematic or lead to unintended behaviour. In matsim-libs/matsim/src/main/java/org/matsim/core/router/NetworkRoutingInclAccessEgressModule.java Line 206 in b487b41
Another point (whith a scope somehow outside of this PR) is that in matsim-libs/matsim/src/main/java/org/matsim/core/router/NetworkRoutingInclAccessEgressModule.java Line 191 in b487b41
|
Hi @d-roeder, I just took a quick look at the code. The |
oh, you're right 👀 It could just be a bit confusing if the toFacility has no link set and NetworkRoutingInclAccessEgressModule then assumes that startLink==endLink. This makes sense in the default logic, but if arbitrary links can be set, then this is no longer necessarily correct. |
Currently, the access/egress link of a car trip is only chosen based on the origin/destination facilities of the trip. We have use cases, where we know the location of a car (at a P&R station), so the "access link" should not be the activity location, but rather the location of the vehicle / station. The generalization of the interface as proposed in this PR allows implementing this kind of custom behavior (for instance by passing additional information through the Attributes of the RoutingRequest).