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:

Status badges

Description (last modified by ncohen)

With this patch CPLEX is not priting the usual screens of output by default :-)

Nathann

Attachments (1)

trac_8880.patch (3.1 KB) - added by ncohen 11 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 11 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 11 years ago by ncohen

  • Summary changed from CPLEX can bi silenced to CPLEX can now be silenced

comment:3 Changed 11 years ago by ncohen

  • Description modified (diff)

comment:4 Changed 11 years ago by jason

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.

comment:5 Changed 11 years ago by jason

(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:

  1. the options are not related to the actual function, so logically they should be separated out from the function options
  2. 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: Changed 11 years ago by ncohen

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 jason

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 jason

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: Changed 11 years ago by 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 ? :-)

Nathann

comment:10 in reply to: ↑ 9 Changed 11 years ago by jason

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 jason

(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: Changed 11 years ago by ncohen

Removes a "CPLEX IS NOT IMPLEMENTED YET" from the solve function's documentation !

comment:13 in reply to: ↑ 12 Changed 11 years ago by jason

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 jason

  • 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 jason

  • Owner changed from jason to ncohen

comment:16 Changed 11 years ago by jason

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 ncohen

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 ncohen

comment:18 Changed 11 years ago by rlm

This is 5 weeks stale by now. Jason-- do you have any further objections to the patch?

comment:19 Changed 11 years ago by ncohen

  • Component changed from numerical to linear programming

comment:20 Changed 11 years ago by ncohen

This ticket can be closed, as the files it modifies are being deleted by #10043

Nathann

comment:21 Changed 11 years ago by mpatel

  • Milestone changed from sage-4.6 to sage-duplicate/invalid/wontfix
  • Resolution set to invalid
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.