Opened 9 years ago

Closed 9 years ago

#7561 closed defect (fixed)

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

Reported by: ncohen Owned by: jkantor
Priority: major Milestone: sage-4.3
Component: numerical Keywords:
Cc: Merged in: sage-4.3.rc0
Authors: Nathann Cohen Reviewers: Martin Albrecht
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by ncohen)

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

Attachments (2)

trac_7561.patch (9.2 KB) - added by ncohen 9 years ago.
trac_7561-review.patch (906 bytes) - added by mhansen 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 9 years ago by ncohen

  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from Replaces InfinitePolynomialRing in MixedIntegerLinearProgram by 'var' to Replaces InfinitePolynomialRing in MixedIntegerLinearProgram by 'var', and bug fixing in constraints()

comment:2 Changed 9 years ago by malb

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

comment:3 follow-up: Changed 9 years ago by ncohen

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

comment:4 Changed 9 years ago by ncohen

Still around ??? :-)

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

comment:5 in reply to: ↑ 3 Changed 9 years ago by malb

Replying to ncohen:

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.

comment:6 Changed 9 years ago by ncohen

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

comment:7 Changed 9 years ago by ncohen

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

Changed 9 years ago by ncohen

comment:8 Changed 9 years ago by malb

  • Status changed from needs_review to positive_review

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

comment:9 Changed 9 years ago by ncohen

Thankssssssssssssss :-)

Changed 9 years ago by mhansen

comment:10 Changed 9 years ago by mhansen

  • Authors set to Nathann Cohen
  • Merged in set to sage-4.3.rc0
  • Resolution set to fixed
  • Reviewers set to Martin Albrecht
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.