Fix floating point comparison in TambyVanderpooten #134
Conversation
src/algorithms/TambyVanderpooten.jl
Outdated
| solutions = [SolutionPoint(X, Y) for (Y, X) in solutions] | ||
| return status, solutions | ||
| solutions_vec = [SolutionPoint(X, Y) for (Y, X) in solutions] | ||
| return status, filter_nondominated(MOI.MIN_SENSE, solutions_vec) |
There was a problem hiding this comment.
Do we need this? In #133, we round the solutions to be integers but I think they are actually different points. Just slightly different due to floating points. Also, solutions is a Dict so the keys should be unique.
There was a problem hiding this comment.
The list of solutions prior to filtering includes stuff like this:
[1277.0, 991.0, 1077.0]
[1277.0, 991.0000000000001, 1077.0]
Maybe we need to change if Y ∉ U_N[u][k] to be approximate
There was a problem hiding this comment.
Yes, that's what I was referring to. We could change it but that would be slower, right? We could allow this and change the test to check for rounded nondominated set. The tradeoff is to allow these approximately equal but ultimately distinct points and pay the price to solve extra scalarizations or check for approximately equality and avoid extra scalarazations. The cost of solving more scalarazations is not perhaps significant for knapsack but it could be for some other problem.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #134 +/- ##
==========================================
- Coverage 98.72% 98.72% -0.01%
==========================================
Files 12 12
Lines 1101 1100 -1
==========================================
- Hits 1087 1086 -1
Misses 14 14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
How's this? |
|
I actually meant that checking for approximate equality could be slower but it will save scalarazations and I missed the fact that some points will be dominated. |
Much cheaper than solving another LP. I tried a similar change for KirlikSayin, but I ran into a bunch of errors (I think because of assumptions to do with the boxes, so I just filtered at the end. |
Closes #133