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 )
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)
Change History (19)
comment:1 Changed 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
- Description modified (diff)
- Keywords Gurobi GurobiBackend MixedIntegerLinearProgram added
comment:3 Changed 7 years ago by
comment:4 Changed 7 years ago by
- 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
- Component changed from numerical to linear programming
comment:6 follow-up: ↓ 7 Changed 7 years ago by
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
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
- Milestone changed from sage-5.0.1 to sage-5.1
comment:9 Changed 7 years ago by
Please fill in your real name as Author.
comment:10 Changed 7 years ago by
comment:11 Changed 6 years ago by
- Reviewers set to Nathann Cohen
comment:12 Changed 6 years ago by
patchbot: apply 16400.patch, trac_12973-review.patch
comment:13 Changed 6 years ago by
- Dependencies set to #14566
Changed 6 years ago by
Changed 6 years ago by
comment:14 Changed 6 years ago by
Rebased !
Nathann
comment:15 Changed 6 years ago by
- Reviewers changed from Nathann Cohen to Volker Braun
- Status changed from needs_review to positive_review
lgtm
comment:16 Changed 6 years ago by
Just learned a new acronym. Thank you very much Volker ! :-)
Nathann
comment:17 Changed 6 years ago by
- Merged in set to sage-5.10.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
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 :
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