#13646: Bug in p.add_constraint (when input is True/False)
-----------------------------------------------+----------------------------
Reporter: ncohen | Owner: ncohen
Type: defect | Status: needs_review
Priority: major | Milestone: sage-5.5
Component: linear programming | Resolution:
Keywords: | Work issues:
Report Upstream: N/A | Reviewers:
Authors: Nathann Cohen, Volker Braun | Merged in:
Dependencies: | Stopgaps:
-----------------------------------------------+----------------------------
Comment (by ncohen):
Volker,
Before anything, please consider that your patches *DO* break a lot of
code I (and some others probably that I do not know) have been using for a
while, and that I have a very hard time keeping from shouting by myself in
my office when I see you do that without caring about those consequences
in any way.
Now that this is said, your patch has good aspects too. So let's talk
about it, without forgetting this important detail.
> Returning `None` as the value of a sum is not cool,
Yeah.... Let's not make a mess of a code just because of that.
> stuff will almost certainly break if you return nothing and not the
neutral element in the module.
Well, please consider that it just *does not*. All the code that you touch
has been running for a while now, it wasn't even a module, and there was
nothing wrong with it, except this ``4 <= variable`` thing. So NO, having
it return None sometimes is not a problem by itself. I don't have to
explain why as you just have to accept that it does not. And I swear this
code has been used.
> `None` is also the value of undefined variables (e.g. typos), this is
rather dangerous.
In this case True/False are the value of undefined linear expressions. No
problem with None. You shouldn't say such things without knowing how this
code is used.
> This is why I switched sum to a method,
Yeah, and it destroys years of source code I have. As I said, I don't mind
much if the changes are good, but it would be very impolite of you to not
inquire about it before sending that patch.
> so that it can return the correct neutral element. As a side effect it
saves you from manually importing `Sum` and it makes it discoverable by
tab completion.
>
> I'll put in a deprecation for `Sum`.
Do we agree if we say that this change with Sum has *NOTHING TO DO* with
the change of base ring ? As you hardcode the basering once, you could
hardcode it twice (once in MILP, another in Sum), and anyway because we
are dealing with RDF and nothing more complicated this Sum may all the
same return int(0) when it is empty for all we care ?
It could even return MixedIntegerLinearProgram.base_ring()[1] for all we
care ?
If these changes are disjoint, please say so. I would be glad to have this
patch only fix the bug reported, and create another ticket for the change
from Sum to p.sum. This latter part is more problematic, I mentionned many
times that it destroyed years of code, but it seems to be a nice change
anyway so why not, after all ?
But it would be nice to do this in a different ticket. If only to be able
to actually see what this patch does with respect to the bug reported
`:-P`
Thank you for your help in solving this thing though. I had no idea how to
do that `:-)`
Nathann
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/13646#comment:19>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
