Skip to content
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

Infinite loop in NetworkTools.findClosestLinksSorted #199

Open
markusstraub opened this issue Feb 2, 2024 · 7 comments
Open

Infinite loop in NetworkTools.findClosestLinksSorted #199

markusstraub opened this issue Feb 2, 2024 · 7 comments

Comments

@markusstraub
Copy link
Contributor

I just wanted to report that NetworkTools.findClosestLinksSorted produces an infinite loop here:

while(tmp.putIfAbsent(d, l) == null) {
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()

double toNodeDist = CoordUtils.calcEuclideanDistance(l.getFromNode().getCoord(), coord);

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

@polettif
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@polettif
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

That's fine for me!

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

No branches or pull requests

3 participants