Opened 11 years ago
Closed 11 years ago
#8880 closed defect (invalid)
CPLEX can now be silenced
Reported by: | ncohen | Owned by: | ncohen |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | linear programming | Keywords: | |
Cc: | Merged in: | ||
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
With this patch CPLEX is not priting the usual screens of output by default :-)
Nathann
Attachments (1)
Change History (22)
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Summary changed from CPLEX can bi silenced to CPLEX can now be silenced
comment:3 Changed 11 years ago by
- Description modified (diff)
comment:4 Changed 11 years ago by
comment:5 Changed 11 years ago by
(I guess my comment is the same here as it is on #8364. I still think it's a better design, but I haven't used these options in practice, so if you think that it is way too annoying, then say so.
My concerns are:
- the options are not related to the actual function, so logically they should be separated out from the function options
- Anytime we implement a new option for the function (say we make a graph-theoretic algorithm to do the same thing), we have to check all solvers to see if we are accidentally overriding a keyword option for the solver.
You could also make the options part of the solver keyword, like this:
edge_cut(solver=("CPLEX", dict(option1=True, option2=False)))
or
edge_cut(solver=dict(option1=True, option2=False)) # pick the default solver, pass in these options.
comment:6 follow-up: ↓ 7 Changed 11 years ago by
Oops... It sounds like the patch uploaded here does not corerespond to the ticket at all ^{};
About #8364, well.. I still agree with you when you say a dict is cleaner, but it will definitely be very quickly annoying to do all this when you just want to change the solver.. And to be honest, there are only 2 different arguments which can be passed down to the MixedIntegerLinearProgram? at the moment :
- log ( verbosity)
- solver (change the default solver)
It may be beter though, to satisfy all of us, to just replace this kwds by these two parameters... So I will completely rewrite #8364 like this if you agree, which needed to be done anyway as its patch is not based on the latest version of sage :-)
(And now I replace the patch contained in this ticket with the good onem you'll see it is much easier to review :-) )
Thank you again !!
Nathann
comment:7 in reply to: ↑ 6 Changed 11 years ago by
Replying to ncohen:
Oops... It sounds like the patch uploaded here does not corerespond to the ticket at all ^{};
Thanks. I was kind of wondering where the CPLEX stuff was... :)
About #8364, well.. I still agree with you when you say a dict is cleaner, but it will definitely be very quickly annoying to do all this when you just want to change the solver.. And to be honest, there are only 2 different arguments which can be passed down to the MixedIntegerLinearProgram? at the moment :
- log ( verbosity)
- solver (change the default solver)
It may be beter though, to satisfy all of us, to just replace this kwds by these two parameters... So I will completely rewrite #8364 like this if you agree, which needed to be done anyway as its patch is not based on the latest version of sage :-)
That sounds like a great plan.
comment:8 Changed 11 years ago by
What about making the options solver_log and solver? That way the log option is a bit more descriptive, and not too much longer than log.
comment:9 follow-up: ↓ 10 Changed 11 years ago by
Hmmm... Ok, but... why ? The user just wants to incraase the level of verbosity of the function he is using, is he really interested in knowing we are using LP in this particular case ? It's as if we were using algorithm_log or function_log... What do we earn this way ? :-)
Nathann
comment:10 in reply to: ↑ 9 Changed 11 years ago by
Replying to ncohen:
Hmmm... Ok, but... why ? The user just wants to incraase the level of verbosity of the function he is using, is he really interested in knowing we are using LP in this particular case ? It's as if we were using algorithm_log or function_log... What do we earn this way ? :-)
Good point. "log" didn't seem to be the usual convention. It would be more conventional to use a "verbose" keyword for printing out lots of intermediate information.
comment:11 Changed 11 years ago by
(I mean that it's Sage's convention to use the verbose keyword for something like that, at least as far as I've seen.)
comment:12 follow-up: ↓ 13 Changed 11 years ago by
Removes a "CPLEX IS NOT IMPLEMENTED YET" from the solve function's documentation !
comment:13 in reply to: ↑ 12 Changed 11 years ago by
Replying to ncohen:
Removes a "CPLEX IS NOT IMPLEMENTED YET" from the solve function's documentation !
Nice. Um, except that the linear programming stuff is not in the "tutorial", but in the "Constructions" document. I can see that being confusing...
Don't update the patch yet; when I get cplex working, I might have another comment or two.
comment:14 Changed 11 years ago by
- Owner changed from jason, jkantor to jason
The cleanup code here should probably be put in a "finally" block, rather than being duplicated. See http://docs.python.org/reference/compound_stmts.html#the-try-statement
comment:15 Changed 11 years ago by
- Owner changed from jason to ncohen
comment:16 Changed 11 years ago by
Even with this patch, the first time I use p.solve(), I see this from CPLEX:
IBM ILOG License Manager: "IBM ILOG Optimization Suite for Academic Initiative" is accessing CPLEX 12 with option(s): "e m b q ".
Is that unavoidable? (Maybe it's desirable, I don't know)
comment:17 Changed 11 years ago by
I had thought during my first tests that this message was a nice way to debug, though it's true the user only needs to incrase the verbosity to see it... Thank you for this "finally" keyword which I ignored :-)
Patch updated.
Nathan
Changed 11 years ago by
comment:18 Changed 11 years ago by
This is 5 weeks stale by now. Jason-- do you have any further objections to the patch?
comment:19 Changed 11 years ago by
- Component changed from numerical to linear programming
comment:20 Changed 11 years ago by
This ticket can be closed, as the files it modifies are being deleted by #10043
Nathann
comment:21 Changed 11 years ago by
- Milestone changed from sage-4.6 to sage-duplicate/invalid/wontfix
- Resolution set to invalid
- Status changed from needs_review to closed
Design-wise, it seems a little uncomfortable to have all extra keywords passed on to the solvers. How annoying would it be to have a solver_opt keyword for options passed down? So something like:
edge_cut(solver_opt=dict(option1=true, option2='solve fast', option3=5))
or
edge_cut(solver_opt={'option1': true, 'option2': 'solve fast', 'option3': 5})
instead of
edge_cut(option1=true, option2='solve fast', option3=5)
It involves a bit more typing, but it seems like a much cleaner design.