search

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
48 stars 66 forks source link

Infinite loop in NetworkTools.findClosestLinksSorted #199

Open markusstraub opened 7 months ago

markusstraub commented 7 months ago

I just wanted to report that NetworkTools.findClosestLinksSorted produces an infinite loop here: https://github.com/matsim-org/pt2matsim/blob/fdfe40a0d5adb4ed98dff4de13662c60cd74dcbb/src/main/java/org/matsim/pt2matsim/tools/NetworkTools.java#L210 while calling this method on a 1km² test map with meter-based projection.

Unfortunately I can not look for a solution any deeper than finding this copy/paste bug (should be l.getToNode() instead of l.getFromNode() https://github.com/matsim-org/pt2matsim/blob/fdfe40a0d5adb4ed98dff4de13662c60cd74dcbb/src/main/java/org/matsim/pt2matsim/tools/NetworkTools.java#L206

.. but even when fixing this the problem does not go away.

polettif commented 7 months ago

Can you share the test map or an extract with the relevant links? I can't really tell what's going on. Mixing to and from nodes doesn't sound good and definitely looks like a bug, I'm surprised it hasn't come up sooner.

markusstraub commented 7 months ago

I tested my code with this test network: https://github.com/ait-energy/matsim-drs/tree/main/data/floridsdorf But I saw that the NetworkTools.findClosestLinksSorted isn't used in pt2matsim anywhere and also has no test coverage - instead NetworkTools.findClosestLinks is used - so maybe the method is an artifact from development long ago?

polettif commented 7 months ago

But I saw that the NetworkTools.findClosestLinksSorted isn't used in pt2matsim anywhere and also has no test coverage - instead NetworkTools.findClosestLinks is used - so maybe the method is an artifact from development long ago?

Yes, sounds like an artifact. I think I realized that there's no use case to get a list sorted by distance (and link direction) without also providing the distance. NetworkTools.findClosestLinks provides the distance in a SortedMap, LinkCandidateCreatorStandard.findClosestLinks then simply picks both links if they're within the threshold. Though I don't know what your application exactly is.

markusstraub commented 7 months ago

I'm now happily using NetworkTools.findClosestLinks, so maybe just remove the method if it is not needed?

polettif commented 7 months ago

Yes, then it's best to remove it. While we're at it, there are probably some other unused or uncovered methods in CoordTools or NetworkTools worth a closer look.

marecabo commented 4 months ago

Hello @markusstraub @polettif,

As I am preparing a new release, I ~fixed~ "adjusted" the method regarding that copy-paste bug but also deprecated it, so we can remove it in a future release.

That does not solve your issue, so I will not close it, but is that fine for you for now?

markusstraub commented 4 months ago

That's fine for me!