search

TimefoldAI / timefold-solver-python

Timefold Solver is an AI constraint solver for Python to optimize the Vehicle Routing Problem, Employee Rostering, Maintenance Scheduling, Task Assignment, School Timetabling, Cloud Optimization, Conference Scheduling, Job Shop Scheduling, Bin Packing and many more planning problems.
https://timefold.ai
Apache License 2.0
36 stars 3 forks source link

fix: Use Number.doubleValue() instead of float(Number) for conversions #75

Closed Christopher-Chianelli closed 3 months ago

Christopher-Chianelli commented 3 months ago

When testing VRP, it was discovered that occasionally, the location, represented as a pair of floats, was converted incorrectly.

It appears JPype's float conversion for Number is inconsistent, so we use Number.doubleValue() instead, which seems to consistency convert correctly.

Ideally, test_float.test_use_64_bits or the other float tests would have catch it, but somehow, those tests never failed; the exact condition for incorrect conversion is unknown.

Christopher-Chianelli commented 3 months ago

Tried float(value.doubleValue()), but that also have the conversion error, which means we cannot really fix the issue without fixing it upstream in JPype.

triceo commented 3 months ago

Tried float(value.doubleValue()), but that also have the conversion error, which means we cannot really fix the issue without fixing it upstream in JPype.

So is this PR a fix or not? And if not, please link the upstream issue.

Christopher-Chianelli commented 3 months ago

Tried float(value.doubleValue()), but that also have the conversion error, which means we cannot really fix the issue without fixing it upstream in JPype.

So is this PR a fix or not? And if not, please link the upstream issue.

The issue still reproduces, but is much more rare with this PR (issue is, type(JDouble) != type(float), hence the test failures)

Exact location of the bug (where the value suddenly change to an unexpected value) is unknown.

Need further investigation.

Christopher-Chianelli commented 3 months ago

Found an alternative solution: not using a distance map avoids the issue (w/o a performance cost)