Skip to content

Fix floating point precision errors when checking location equality#92

Closed
sorig wants to merge 1 commit intonextmv-io:developfrom
sorig:location-equality-checks
Closed

Fix floating point precision errors when checking location equality#92
sorig wants to merge 1 commit intonextmv-io:developfrom
sorig:location-equality-checks

Conversation

@sorig
Copy link
Copy Markdown

@sorig sorig commented Jun 12, 2025

Another quick PR that's more straightforward than #91

Direct floating point comparisons are affecting the unplan operator when it tries to unplan stops at the same location.

I haven't done proper tests of how this changes performance (I guess it is possible that the added randomness aids the search in some scenarios)

@nmisek nmisek requested a review from merschformann June 12, 2025 17:09
@nmisek
Copy link
Copy Markdown
Contributor

nmisek commented Jun 12, 2025

Thank you @sorig - looping in @merschformann to review

@merschformann
Copy link
Copy Markdown
Member

Thank you for the contribution @sorig !

Can you explain the issues in some more detail? As far as I am aware latitude and longitude are untouched values during search, so, equality of two stops should stay the same / be deterministic. I might be missing something though.

The tolerance comparison may make sense anyway though, as input data may have some nervousness around the last digits. I can't see any performance degradation due to the change in a first benchmark either.

I'll discuss tomorrow with a colleague again and potentially just merge. 😊

Again, thanks for the contribution! 🤗

@sorig
Copy link
Copy Markdown
Author

sorig commented Jun 13, 2025

Can you explain the issues in some more detail? As far as I am aware latitude and longitude are untouched values during search, so, equality of two stops should stay the same / be deterministic. I might be missing something though.

Hmm I think you're right. It's possible I misinterpreted the behaviour I was seeing. I noticed this behaviour through an interactive debugging session where the unplan operator didn't seem to fully unplan all stops at the same location at the same time in solve_operator_unplan.go>unplanLocation(). This was a little while ago and I didn't produce a minimal example of it. It is possible that I mistook the other weird unplanning behaviour in #91 for this issue.

I'll try to see if I can reproduce the behaviour I saw.

@merschformann
Copy link
Copy Markdown
Member

Thank you @sorig for you time and effort! ❤️

We'll be looking into the other one as well. 😊

@merschformann
Copy link
Copy Markdown
Member

Just FYI @sorig : we looked into this a bit today. We can't merge the proposed changes, as they are breaking the alternate and stop group feature. However, we plan to look into this more closely and start from here. 😊

@sorig sorig closed this Jul 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants