Improve documentation of constraint_generation in MixedIntegerLinearProgramming

[sagetrac-equaeghe]

As prompted by http://ask.sagemath.org/question/3410, this ticket is meant for discussion to improve the documentation of the constraint_generation option of MixedIntegerLinearProgramming.

From the ref: http://www.sagemath.org/doc/reference/numerical/sage/numerical/mip.html:

constraint_generation – whether to require the returned solver to support constraint generation (excludes Coin). False by default.

From the source: https://github.com/sagemath/sage-prod/blob/master/src/sage/numerical/backends/generic_backend.pyx#L983:

• constraint_generation (boolean) -- whether the solver returned is to be used for constraint/variable generation. As the interface with Coin does not support constraint/variable generation, setting constraint_generation to False ensures that the backend to Coin is not returned when solver = None. This is set to False by default.

First issue: in the source the phrase “setting constraint_generation to False ensures that the backend to Coin is not returned when solver = None” seems incorrect, as setting constraint_generation to False is meant to allow selection of Coin.

Second issue: upon first reading the ref and source documentation, I understood that Coin would not be selected if constraint_generation were set to True, but not what was exactly meant by ‘constraint generation’ in the context of the module. I thought it would enable passing a constraint generator. Now I understand that constraint generation means: after a first call solve(), one or more constraints is added to the solved instance, and then solve() is called again. Perhaps the documentation should be changed to make this clearer.

Component: numerical

Author: Erik Quaeghebeur

Branch/Commit: d0b48c6

Reviewer: Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/15686

[nathanncohen]

comment:2

Hmmmmmmm... Well I agree with the first False should be changed into True, but if you want any other change perhaps you should write those lines yourself ?.. ^^;

Nathann

[sagetrac-equaeghe]

comment:3

Replying to @nathanncohen:

Hmmmmmmm... Well I agree with the first False should be changed into True, but if you want any other change perhaps you should write those lines yourself ?.. ^^;

Of course. Perhaps we keep this ticket open after issue 1 is fixed to allow me to provide the changed lines if I get around to it in the future.

[nathanncohen]

comment:4

Well if you just want to change a couple of lines in the doc perhaps we can wait until you find the time ? :-P

[sagetrac-equaeghe]

comment:5

Replying to @nathanncohen:

Well if you just want to change a couple of lines in the doc perhaps we can wait until you find the time ? :-P

You forced my hand:

https://github.com/equaeghe/sage/compare/patch-1

https://github.com/equaeghe/sage/compare/patch-2

Do I create pull requests for these, or what is the preferred workflow?

[nathanncohen]

comment:6

Hellooooooo !!

You forced my hand:

Ahahah. Well, wasn't the only alternative for me to rewrite a doc that I already find clear ? :-P

Do I create pull requests for these, or what is the preferred workflow?

Well, depending how you are used to git, the workflow is more or less simple :

• Get the Trac server to know your public ssh key
• Modify Sage's source code, and upload the commits as a branch
• Change the "branch" field of this ticket if it is not done automatically.

It is all explained there [1], and I can help with any specific question you have if it isn't clear. And we will update the developper's documentation if something isn't clear, too ^^;

By the way, your changes as I see them are not correct : When constraint_generation is set to False, it may very well be that the solver returned does supports constraint generation. That will actually happen whenever Coin is not installed on the machine.

And I don't know if it is a good idea to mention Coin explicitly in the docstring, for there may come a time when another solver that does not support constraint_generation may be made available in Sage, and it's quite likely we will forget to update the documentation when the code is changed... But well :-P

Nathann

[1] http://www.sagemath.org/doc/developer/

[sagetrac-equaeghe]

Commit: 44c49ca

[sagetrac-equaeghe]

New commits:

44c49ca

first try for trac issue #15686:

[sagetrac-equaeghe]

Branch: u/equaeghe/15686

[sagetrac-equaeghe]

comment:8

Replying to @sagetrac-equaeghe:

New commits:

44c49ca

first try for trac issue #15686:

I still have some trouble building the html/pdf reference on my end (sage-on-gentoo...), so I couldn't check that yet.

[nathanncohen]

comment:9

Hellooooooooo !!

As I said earlier, you cannot say that a solver returned when calling constraint_generation=False does not support constraint generation. You can only say that it does support constraint generation when constraint_generation is set to True. Also, as far as I know you can only add constraints, not modify them.

Nathann

[sagetrac-equaeghe]

comment:11

Replying to @nathanncohen:

As I said earlier, you cannot say that a solver returned when calling constraint_generation=False does not support constraint generation. You can only say that it does support constraint generation when constraint_generation is set to True.

Ok.

Also, as far as I know you can only add constraints, not modify them.

Hmm, what exactly doesn't CBC support: adding constraints and resolving of course, but what about removing constraints, changing variable bounds, changing the objective?

[nathanncohen]

comment:12

Hello !

Also, as far as I know you can only add constraints, not modify them.

Hmm, what exactly doesn't CBC support: adding constraints and resolving of course, but what about removing constraints, changing variable bounds, changing the objective?

I don't know exactly. My use of LP is pretty simple : I add constraints and solve them. Sometimes, I then add constraints and solve them another time. What I can swear is that I cannot so the following with Coin, and that they told me it was not supported.

Removing constraints or modifying them is what I consider a "fancy use" of LP already. I mean, these things are meant to solve sets of equations, they are not text editors :-P

Nathann

[nathanncohen]

comment:13

Well, to give you a more serious answer I don't know, and I think the answer is in their documentation. Though if you cannot add constraints you probably can't change them either, for then to "add" constraints you could add empty constraints when the LP is built, and modify them into real constraints to "mimic" constraint generation. And if they do not support it they probably have a technical problem that prevents them from doing it.

Nathann

[sagetrac-git]

Changed commit from 44c49ca to f220172

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f220172

second try for trac issue #15686

[nathanncohen]

comment:16

Hellooooo !

Sorry but I do not find this much clearer. Instead of repeating many times "when solver=None" couldn't you add in the documentation of the solver parameter that "when it is define the value of constraint_generation is ignored" ? But I don't know what a guy specifying both a solver and a value to constraint_generation would expect O_o

Nathann

[sagetrac-git]

Changed commit from f220172 to d0b48c6

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d0b48c6

third try for trac issue #15686

[sagetrac-equaeghe]

comment:19

Replying to @nathanncohen:

Sorry but I do not find this much clearer. Instead of repeating many times "when solver=None" couldn't you add in the documentation of the solver parameter that "when it is define the value of constraint_generation is ignored" ?

I've made a modification much like the one you suggest, but in the constraint_generation documentation

But I don't know what a guy specifying both a solver and a value to constraint_generation would expect O_o

Well, currently, as far as I can tell from the code, the selected solver is used. Only if solver=None is the constraint_generation parameter taken into account. The current reformulation is compatible with this.

[nathanncohen]

Reviewer: Nathann Cohen

[nathanncohen]

comment:20

Okayyyyyyyyyyy let's get it in then. This has waited long enough :-P

Please add your name to the "Author" field.

Nathann

[sagetrac-equaeghe]

Author: Erik Quaeghebeur

[vbraun]

Changed branch from u/equaeghe/15686 to d0b48c6