-
-
Notifications
You must be signed in to change notification settings - Fork 519
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
Wrong result from delsarte_bound_additive_hamming_space with GLPK exact simplex #20447
Comments
comment:1
(See #20406 where this was discovered.) |
Dependencies: #20406 |
comment:3
the function performs the following, in a loop: maximises the sum of variables, gets the value of this maximum, say M, then rounds M down to the closest m:=7d, adds a constraint that the sum of the variables is at most m, and repeats. In the case of rational variables, this process obviously must stop after one iteration (it need not stop if all the variables are integers). When it stops, it returns the latest d.
here it should have been done, and return 3 (as 73=343), but it is not!
|
comment:4
in its infinite wisdom, GLPK returns the result of an exact computation as a float. And then the line
compares, for equality, the exact value, 343=73, with the float 342.999999999999943, and founds them unequal. Whereas for the correct functioning of the algorithm, they must be equal. Not sure whether this is an upstream bug, or an upstream feature... |
Upstream: Not yet reported upstream; Will do shortly. |
comment:5
How come you never run into trouble with the floating-point based solvers with this kind of code? It is certainly ironic that the exact solver has more floating-point fuzz than the floating-point solvers. But your code cannot be correct if solver is a floating-point solver. And there is an apparent upstream issue with GLPK. I wouldn't call it a bug, but given the design decision to return the exact values as double-floats, it should at least be improved to return all integers that have an exact representation in double-float as such. |
comment:6
Replying to @mkoeppe:
of course I did, and docs explicitly say that as soon as you specify another solver, you are on your own.
I don't think there is an API to check whether a solver is exact, and so I never bothered to check this in the code.
this design is called lazyness, in CS, and not only :-) |
comment:7
Replying to @dimpase:
You can query the |
comment:8
Replying to @mkoeppe:
well, Besides, I don't think there are places in Sage that forbid doing things cause they are "risky". |
comment:9
Replying to @dimpase:
Perhaps you mean a
I think there more places should forbid things like that, or at least display a warning. For example, the polyhedral code in Sage is written in a way that it assumes exact arithmetic -- and if fed with floating point numbers, leads to mysterious errors. One would usually assume from algorithms that accept floating point numbers that they have some (however naive) accommodation for floating point fuzz, in the form of some epsilons. |
comment:10
The relevant code is in GLPK's glpapi07.c; it's using |
comment:11
Actually, storing the rational results as doubles is fine, in fact I have a test in So there's no upstream bug to be reported -- except that we still really want GLPK to make the interface to glpssx.h public (#18765). Guarding your code against floating-point fuzz is a wishlist item -- I'm marking this ticket as such. |
Changed upstream from Not yet reported upstream; Will do shortly. to none |
comment:12
So isn't this essentially a duplicate of #18765 then? |
comment:13
No, #18765 is about making a "proper" rational GLPK API available. I've changed the description of the present ticket to say what should be done. |
delsarte_bound_additive_hamming_space
should be guarded, using epsilons, against floating point fuzz that will appear with numerical solvers and even the GLPK exact solver because of the nature of its interface.Depends on #20406
CC: @dimpase
Component: coding theory
Issue created by migration from https://trac.sagemath.org/ticket/20447
The text was updated successfully, but these errors were encountered: