matsim-org / pt2matsim

Package to create a multi-modal MATSim network and schedule from public transit data (GTFS or HAFAS) and an OSM map of the area.
http://www.ivt.ethz.ch/publikationen/studenten/530.html
GNU General Public License v2.0
49 stars 68 forks source link

Support for lane based network routing #189

Open auzpatwary37 opened 1 year ago

auzpatwary37 commented 1 year ago

Lanes are pertinent tools to model turn restrictions. Given original MATSim has a lane-based routing and given turns are vital to the routing of transit vehicles, it is only fitting to extend the functionality of lane-based routing in pt2MATSim from the core MATSim.

I have tried to implement such. The problems I faced were:

  1. Access to the main Config and corresponding config modules are restricted to almost all classes inside pt2MATSim.
  2. The InvertedLeastCostPathClaculator in MATSim is, for some reason, not a public class, requiring the implementation of the same class yet again inside pt2MATSim.

It is still a work in progress. Please advise if you have any suggestions.

auzpatwary37 commented 1 year ago

I have completed a version allowing for lane-based routing and pushed it to a forked repo with version no 23.4_r. This version passes the original tests, allowing for minor modifications in the function inputs due to network inversion (allowing access to the main config to most classes and using links instead of nodes while calculating the least cost paths). I have not implemented it yet on any large-scale network for transit mapping. But that is the next phase. Please let me know if you have any concerns or suggestions. Feel free to clone and test out the code. Thanks.

Here is the link for the forked repo:

https://github.com/auzpatwary37/pt2matsim.git

polettif commented 1 year ago

Thanks for tackling this issue and improving the package! I had a quick look into your fork but I think a review is best done with a pull request. Feel free to open one when you think the code is ready.

Something I noticed while looking through the code: You pass a new config (object or file path), mostly called mainConfig, in a lot of the methods, what's the reason for this?

auzpatwary37 commented 1 year ago

Hi, thanks for the reply. The primary purpose was to take the lane definition file location while creating the InvertedLeastCostPathCalculator class which is already a part of the original config. The network is also a part of it, so I did not get why the network file location was taken as an input in the ptMapperConfigGroup. As the config has the ptMapperConfigGroup as part of it, my final intention was to pass the main config rather than the ptMapperConfigGroup to most classes.

polettif commented 1 year ago

Ah I see. Wouldn't it be easier to extend the ptMapperConfigGroup with a parameter for the lane definition file?

auzpatwary37 commented 11 months ago

@polettif As I am new to modifying MATSim code, I was trying for minimal modifications. As the necessary information is already in the config file, which also contains any additional modules, I was trying to push that inside. I have worked on three different parts: first, adding lane-based routing; second, adding support to extract lane information and lane restrictions from OSM; and finally, I have combined a file to combine the two logics to create a network generation workflow. As I am new in coding, I would appreciate some advice on raising pull requests for specific features. I am planning to create a different branch that is synced to the original and then push each feature one by one. I am sorry for the late response.

polettif commented 11 months ago

@polettif As I am new to modifying MATSim code, I was trying for minimal modifications. As the necessary information is already in the config file, which also contains any additional modules, I was trying to push that inside.

The issue with this approach is that you need to change function arguments throughout the codebase: Wherever a ptMapperConfigGroup config is required you need to add another parameter. You shouldn't really use a general MATSim simulation config file for pt2matsim as pt2matsim's functions only use the PublicTransitMappingConfigGroup. In a way we just use the config class to pass on the mapping configuration.

Now, if you want to avoid duplicating config info (I don't know how many parameters are needed for InvertedLeastCostPathCalculator) you could also pass the whole config file, especially while developing. You'd still need to define (and document) which parameters of which modules are used though.

I have worked on three different parts: first, adding lane-based routing; second, adding support to extract lane information and lane restrictions from OSM; and finally, I have combined a file to combine the two logics to create a network generation workflow.

That would be a great addition to pt2matsim. It's quite a task to do though, even for one of those parts.

As I am new in coding, I would appreciate some advice on raising pull requests for specific features. I am planning to create a different branch that is synced to the original and then push each feature one by one. I am sorry for the late response.

I'd suggest you tackle these steps:

You can develop each feature on your own git fork. Don't forget adding unit tests. After you've developped a feature you can create a pull request so we can review it and add it to pt2matsim. If the modules are set up correctly you don't really need a new workflow as the upgraded modules fit right within the exiting pt2matsim workflow (by updating some default classes).