matsim-libs
matsim-libs copied to clipboard
matsim-org
Promote PtNetworkSimplifier to org.matsim package
With this PR, I'd suggest to move the PtNetworkSimplifier to the org.matsim package.
Recently, I used the PtNetworkSimplifier from the vsp playground a lot and it worked well as expected. Credits and many thanks to aneumann (is it @neuma?) for it.
I changed and added a few things:
- Made the API more compatible to the one of the regular
NetworkSimplifier - Parallelized some checks for improved performance on large pt networks.
- Wrote a test case
Thanks for your suggestion. @vsp-gleich do you have an oppinion on this? Otherwise this looks fine for merging to me.
This class can be useful for models in which pt is sharing the network with car and other modes. Usually we have pt on a separate pseudo network for which no simplification should be possible. So, this is for now not the most essential functionality which must reside in the matsim core. But I'm not really against moving it either. It would be nice to test the transit schedule modifications in the test and maybe test run any of the typical pt pseudo networks (e.g. from any open scenario) and see whether possible simplifications are reported/made (in my intuition there shouldn't be any simplification possible). In comparison to the standard NetworkSimplifier this PtNetworkSimplifier only cares about NetworkTopoType and whether links with pt services are affected. Maybe some kind of delegation to the standard NetworkSimplifier would be better on the long run? The PtNetworkSimplifier only adds the check for pt services feature and the modification of affected pt services. What do you think @Janekdererste ?
Background info: I used it actually to create pseudo-pt networks from networks generated with PT2MATSim. There, pt routes are mapped to OSM ways (while keeping their link geometries) and simplification was really helpful. If you do not need your pseudo-pt network to have real link geometries, then you can just create the pseudo-pt network from the schedule alone and don't need simplification.
Background info: I used it actually to create pseudo-pt networks from networks generated with PT2MATSim. There, pt routes are mapped to OSM ways (while keeping their link geometries) and simplification was really helpful. If you do not need your pseudo-pt network to have real link geometries, then you can just create the pseudo-pt network from the schedule alone and don't need simplification.
That makes sense, thank you for explaining! Actually when using pt2matsim to map stops to osm ways wouldn't it be helpful to then have the same simplification functions as the standard NetworkSimplifier? Especially if buses shall share the normal car network (since they are already mapped to it and road congestion effects are nice to have)?
What do you think @Janekdererste ?
I would not automatically call the network simplifier from here. If someone is interestend in doing both, I would think it is better to do so explicitly. I am not sure whether I completely understood your suggestion though 🙃.
Thank you for adding a test for the modified TransitRoute! The vsp contrib version of PtNetworkSimplifier should be deleted during the move. EDIT: Remove phrase on apparently deleted TransitScheduleCleaner reference.
What do you think @Janekdererste ?
I would not automatically call the network simplifier from here. If someone is interestend in doing both, I would think it is better to do so explicitly. I am not sure whether I completely understood your suggestion though upside_down_face.
NetworkSimplifier and PtNetworkSimplifier have quite some overlaps (e.g. method bothLinksHaveSameLinkStats() and parts of the run() method), but neither can fully replace the other. This opens questions for code maintainability and end users. I fear some users will not be aware of both versions and their limitations, and will e.g. use NetworkSimplifier without thinking about consequences for pt services or re-implement features in PtNetworkSimplifier that already exist in NetworkSimplifier (e.g. more constraint/filter options to chose which links can be merged). That's why maybe merging the PtNetwork stuff into NetworkSimplifier might be better.
That's why maybe merging the PtNetwork stuff into NetworkSimplifier might be better.
Now, that you've explained this to me offline, I do agree with your preference to merge the pt-simplifier into the regurlar NetworkSimplifier.
@marecabo would you be willing to do that instead?
@vsp-gleich Thanks for the discussion last week. When I find some time to merge both, can I get rid of the these parts from the NetworkSimplifier, as they run method allowing to vary thresholdLength is already marked as deprecated?
- https://github.com/matsim-org/matsim-libs/blob/2c5cd05b2ef82bef03eb000ebc812ba5e940b748/matsim/src/main/java/org/matsim/core/network/algorithms/NetworkSimplifier.java#L101-L116
- https://github.com/matsim-org/matsim-libs/blob/2c5cd05b2ef82bef03eb000ebc812ba5e940b748/matsim/src/main/java/org/matsim/core/network/algorithms/NetworkSimplifier.java#L161-L181
- https://github.com/matsim-org/matsim-libs/blob/2c5cd05b2ef82bef03eb000ebc812ba5e940b748/matsim/src/main/java/org/matsim/core/network/algorithms/NetworkSimplifier.java#L223-L234
The latter two parts have no function if someone uses a thresholdLength = positive infinity as recommended in Kai's comment.
Hi @marecabo , thanks for taking up my suggestion!
I tend to say yes you can remove the deprecated method, but please keep Kai's comments somehow. The NetworkSimplifierTest also uses the deprecated run method, so removing the deprecated run method will cause some work over there too.
What is the status of this PR @vsp-gleich @marecabo? Could we finish this one?
