Opened 7 years ago

Closed 6 years ago

#12973 closed defect (fixed)

Cannot copy() instances of GurobiBackend.

Reported by: e.vaughan Owned by: jason, jkantor
Priority: minor Milestone: sage-5.10
Component: linear programming Keywords: Gurobi, GurobiBackend, MixedIntegerLinearProgram
Cc: Merged in: sage-5.10.beta3
Authors: Emil R. Vaughan, Nathann Cohen Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14566 Stopgaps:

Description (last modified by ncohen)

GurobiBackend? does not implement copy().

I've attached a patch to add this functionality.

The patch also corrects a few doctests in the same file, that have solver="Gurobi" instead of solver="GUROBI".

(The patch was created with Sage 4.8.)

APPLY:

Attachments (2)

16400.patch (2.3 KB) - added by ncohen 6 years ago.
trac_12973-review.patch (1005 bytes) - added by ncohen 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by e.vaughan

  • Description modified (diff)

comment:2 Changed 7 years ago by e.vaughan

  • Description modified (diff)
  • Keywords Gurobi GurobiBackend MixedIntegerLinearProgram added

comment:3 Changed 7 years ago by ncohen

Wow !! Very good patch for a first ! :-D

Well, it did not apply cleanly on version 5.1.beta0 but that was to be expected if you worked on 4.8 Actually, the replacement from GUROBI to Gurobi has been done since, and that was the whole reason why it did not apply. I just rebased your patch on 5.1.beta0 and updated it on the trac ticket.

There is a problem though, for your patch does not *compile*. When I apply it and type "sage -b" I get this :

Error compiling Cython file:
------------------------------------------------------------
...
            sage: p.set_objective([2, 5])                                           # optional - Gurobi
            sage: p.write_lp(SAGE_TMP+"/lp_problem.lp")                             # optional - Gurobi
        """
        check(self.env, GRBwrite(self.model[0], filename))

    cpdef GurobiBackend copy(self):
         ^
------------------------------------------------------------

sage/numerical/backends/gurobi_backend.pyx:1079:10: C method 'copy' not previously declared in definition part of extension type
Error running command, failed with status 256.
Error installing modified sage library code.

This is because cdef methods should be added to gurobi_backend.pxd as it is added for glpk in glpk_backend.pxd. I will post another patch to do exactly that.

Nathann

comment:4 Changed 7 years ago by ncohen

  • Description modified (diff)
  • Status changed from new to needs_review

Nice !

So I agree with your changes, but as I added something myself you also need to check that everything is fine on your side, and then we will be able to set this ticket to "positive review", which means that the release manager can add it to the next release of Sage!

Thank you for your patch !!

Nathann

comment:5 Changed 7 years ago by ncohen

  • Component changed from numerical to linear programming

comment:6 follow-up: Changed 7 years ago by e.vaughan

Thanks Nathann! I am a bit reluctant to upgrade to Sage 5.x to test the patch due to the issues with Core2 Duo Macs....

Also, in my original patch, I *did* have a

cpdef GurobiBackend copy(self)

line.... Maybe that bit of the patch didn't apply properly in 5.1?

One question: why have you changed copy to be a cdef instead of a cpdef? (I used cpdef because that's what GLPKBackend uses (or did in 4.8).)

comment:7 in reply to: ↑ 6 Changed 7 years ago by ncohen

Helloooooo !!!

Thanks Nathann! I am a bit reluctant to upgrade to Sage 5.x to test the patch due to the issues with Core2 Duo Macs....

Ahaha. That's understandable :-D So the bug happens even when you compile from source ? Scary mac..

Also, in my original patch, I *did* have a

cpdef GurobiBackend copy(self)

line.... Maybe that bit of the patch didn't apply properly in 5.1?

OOpps. Yes, probably. I looked at the list of rejected lines and saw so many GUROBI -> Gurobi, I probably missed that one.

One question: why have you changed copy to be a cdef instead of a cpdef? (I used cpdef because that's what GLPKBackend uses (or did in 4.8).)

Oops, you are right. Actually, cpef functions are functions that can be called at either C-level or Python level. Well, it more or less changes nothing in this situation, as the code onl calls the copy method from the backends at C level. It's actually totally up to you :-)

Nathann

comment:8 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.0.1 to sage-5.1

comment:9 Changed 6 years ago by jdemeyer

Please fill in your real name as Author.

comment:10 Changed 6 years ago by ncohen

  • Authors set to Nathann Cohen

comment:11 Changed 6 years ago by ncohen

  • Authors changed from Nathann Cohen to Emil R. Vaughan
  • Reviewers set to Nathann Cohen

comment:12 Changed 6 years ago by vbraun

patchbot: apply 16400.patch, trac_12973-review.patch

comment:13 Changed 6 years ago by vbraun

  • Dependencies set to #14566

Changed 6 years ago by ncohen

Changed 6 years ago by ncohen

comment:14 Changed 6 years ago by ncohen

Rebased !

Nathann

comment:15 Changed 6 years ago by vbraun

  • Authors changed from Emil R. Vaughan to Emil R. Vaughan, Nathann Cohen
  • Reviewers changed from Nathann Cohen to Volker Braun
  • Status changed from needs_review to positive_review

lgtm

comment:16 Changed 6 years ago by ncohen

Just learned a new acronym. Thank you very much Volker ! :-)

Nathann

comment:17 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.10.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.