Skip to content
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

Replaces InfinitePolynomialRing in MixedIntegerLinearProgram by 'var', and bug fixing in constraints() #7561

Closed
nathanncohen mannequin opened this issue Nov 30, 2009 · 13 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Nov 30, 2009

InfinitePolynomialRing was responsible for some bugs and extreme slowness in the utilisation of MixedINtegerLinearProgram for LP containing more than 1000 variables.

By replacing this polynomial ring by 'var', this is settled and waaaaayyyy mroe efficient !!

One simple bug in constraints() is also fixed in this patch. A nasty -1 was shifting all the constraints compared to their bounds. This only affected the functions show() and constraints() and is of no incidence on the solve() function.

This patch depends on #7270

Component: numerical

Author: Nathann Cohen

Reviewer: Martin Albrecht

Merged: sage-4.3.rc0

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

@nathanncohen nathanncohen mannequin added this to the sage-4.3 milestone Nov 30, 2009
@nathanncohen

This comment has been minimized.

@nathanncohen nathanncohen mannequin changed the title Replaces InfinitePolynomialRing in MixedIntegerLinearProgram by 'var' Replaces InfinitePolynomialRing in MixedIntegerLinearProgram by 'var', and bug fixing in constraints() Dec 1, 2009
@nathanncohen nathanncohen mannequin added the s: needs review label Dec 1, 2009
@malb
Copy link
Member

malb commented Dec 2, 2009

comment:2

Patch looks good and indeed speeds up everything considerably for me. Some small issues:

  • id should not be used as a variable name
  • The documentation says # Returns the optimal value of x[3] but you are asking for y[2][9]
  • min/max should not be used as variable names

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 2, 2009

comment:3

Hello !!! It seems there are no "id" anymore in the file, and I corrected the x[3]..

The min/max variables are only defined in some (short) functions where they appear natural enough... Do you think we should change the arguments of the add_constraint() function ? I understand why you do not like to see this and I was not aware of it before you mentionned it, but they seem mostly harmless and I have no idea of which keyword we could pick to replace them... If you have any idea, though.... :-)

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 5, 2009

comment:4

Still around ??? :-)

It would be good if this patch was merged into next release ! :-)

@malb
Copy link
Member

malb commented Dec 5, 2009

comment:5

Replying to @nathanncohen:

Hello !!! It seems there are no "id" anymore in the file, and I corrected the x[3]..

Hi, it seems you didn't upload your new patch.

The min/max variables are only defined in some (short) functions where they appear natural enough... Do you think we should change the arguments of the add_constraint() function ? I understand why you do not like to see this and I was not aware of it before you mentionned it, but they seem mostly harmless and I have no idea of which keyword we could pick to replace them... If you have any idea, though.... :-)

How about minimum & maximum? I'll leave it to you to change it or to keep min/max.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 5, 2009

comment:6

I forgot to uploaded it and erased it since... I'll upload a new one in a few seconds ! :-)

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 5, 2009

comment:7

Here it is ! I only corrected the x[3] and let the min/max be... The function in which they appear is very short, and longer version would mean updating manymany patches (currently under review) and having longer aliases when it does not hurt that much. ( though I understand how you felt about them, I did not realized it when I first used them ).

We will be able to change them later if needed anyway :-)

Nathann

@malb
Copy link
Member

malb commented Dec 5, 2009

comment:8

Attachment: trac_7561.patch.gz

This should go into 4.3, it opens a whole new world for MIP problems.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Dec 5, 2009

comment:9

Thankssssssssssssss :-)

@mwhansen
Copy link
Contributor

mwhansen commented Dec 6, 2009

comment:10

Attachment: trac_7561-review.patch.gz

@mwhansen
Copy link
Contributor

mwhansen commented Dec 6, 2009

Reviewer: Martin Albrecht

@mwhansen
Copy link
Contributor

mwhansen commented Dec 6, 2009

Author: Nathann Cohen

@mwhansen
Copy link
Contributor

mwhansen commented Dec 6, 2009

Merged: sage-4.3.rc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants