Opened 11 years ago

Closed 11 years ago

#9269 closed defect (fixed)

clean up #optional tags in sage/graphs

Reported by: rlm Owned by: mvngu
Priority: major Milestone: sage-4.5
Component: documentation Keywords:
Cc: Merged in: sage-4.5.alpha1
Authors: Robert Miller Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Attachments (2)

trac_9269.patch (37.6 KB) - added by rlm 11 years ago.
trac_9269-part2.patch (10.1 KB) - added by rlm 11 years ago.

Download all attachments as: .zip

Change History (13)

Changed 11 years ago by rlm

comment:1 Changed 11 years ago by rlm

  • Status changed from new to needs_review

Changed 11 years ago by rlm

comment:2 Changed 11 years ago by jhpalmieri

Why has CPLEX been removed from some tags and not others?

comment:3 Changed 11 years ago by ncohen

  • Status changed from needs_review to positive_review

Cplex has been left where it was explicitely required : for the method solve_cplex for example, which is only compiled when CBC has been compiled with Cplex itself.

Its other occurences are of a different kind : I named it as it was one of the three solver available, though there is no way to install Cplex in Sage without installing Cbc before :-)

Thank you Robert !!!

Nathann

comment:4 follow-up: Changed 11 years ago by jhpalmieri

Two comments: first, this won't work with -only-optional without the patch from #9272: any line marked # optional - TAGS in which TAGS contains capital letters can't be tested with -only-optional because of a bug in sage-doctest. So was this actually tested? I'm suspicious...

Second, lines marked # optional - GLPK, CBC require -only-optional=GLPK,CBC to run: using -only-optional=GLPK won't run them. That is, if you include several tags on an "optional" line, they are combined using "and", not "or". I don't think that is what's intended here. Should we leave it as is, or change it somehow? Or at least document it somewhere in the file?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 11 years ago by rlm

Replying to jhpalmieri:

Two comments: first, this won't work with -only-optional without the patch from #9272: any line marked # optional - TAGS in which TAGS contains capital letters can't be tested with -only-optional because of a bug in sage-doctest. So was this actually tested? I'm suspicious...

I did test this, and I found something slightly different from what you claim: the script takes the tags in the doctest and converts them to lower case, while not doing the same for the command line arguments. So the combination # optional - BLAH plus -only-optional=blah works, while neither # optional - BLAH plus -only-optional=BLAH nor # optional - blah plus -only-optional=BLAH works.

Second, lines marked # optional - GLPK, CBC require -only-optional=GLPK,CBC to run: using -only-optional=GLPK won't run them. That is, if you include several tags on an "optional" line, they are combined using "and", not "or". I don't think that is what's intended here. Should we leave it as is, or change it somehow? Or at least document it somewhere in the file?

In my recent sage-devel post, I mentioned that there is no support for OR in this scheme. My solution was to have doctests requiring GLPK or CBC to have both listed, and to use -only-optional=glpk,cbc when either is installed.

I meant to remove CPLEX from *all* the tests, since there is no cplex package.

comment:6 in reply to: ↑ 5 Changed 11 years ago by jhpalmieri

Replying to rlm:

I did test this,

(Yes, but you weren't the reviewer.)

and I found something slightly different from what you claim: the script takes the tags in the doctest and converts them to lower case, while not doing the same for the command line arguments. So the combination # optional - BLAH plus -only-optional=blah works, while neither # optional - BLAH plus -only-optional=BLAH nor # optional - blah plus -only-optional=BLAH works.

You're right, I mixed this up here (but I think I got it right on #9272).

In my recent sage-devel post, I mentioned that there is no support for OR in this scheme. My solution was to have doctests requiring GLPK or CBC to have both listed, and to use -only-optional=glpk,cbc when either is installed.

Right, my question is whether we should put a comment about this at the top of the affected files, or do we just trust people to know how to use "-only-optional"?

I meant to remove CPLEX from *all* the tests, since there is no cplex package.

I think this flag probably still belongs in mip_cplex.pyx, if no place else. Someone might have CPLEX installed separately from Sage, and having a mechanism to test is not a bad idea. ("optional" tags don't need to correspond to packages, like # optional - internet or # optional - Mathematica.)

comment:7 Changed 11 years ago by ncohen

Hmmm.... It looks like I was much less attentive to this patch than you were O_o

Actually, I just trusted the problem was that an #optional comment must only contain this very keyword, followed by the names of the packages, and that this motivated the whole patch.

Just in case you wonder, when I write patches using LP, I usually use -optional without any argument, so that all optional packages are tested. Besides, I do not like to see all the optional flags pass successfully the first time (exactly because I do nt know if all of them were tested), so when it happens I usually change the result of a command to see whether Sage "sees" it :-)

Do yo think this patch needs to be modified, John ?

Nathann

comment:8 Changed 11 years ago by jhpalmieri

Do yo think this patch needs to be modified, John ?

I'm guessing that you have either GLPK or CBC installed, or both. Could you just test that "sage -t -only-optional=glpk,cbc ..." works on the affected files? In more detail:

  • "sage -t -only-optional=glpk ..." should finish instantly, because it shouldn't run any tests.
  • "sage -t -only-optional=glpk,cbc ..." should *not* finish instantly, and all tests should pass.

I've checked this without GLPK and CBC installed, and the second of these fails a bunch of tests, as it should.

comment:9 Changed 11 years ago by ncohen

Hello !!!

It is exactly as you said ! Well, except for several errors in generic_graph.py, but this is just because of the recent NetworkX update and is already fixed in #9230.

Nathann

comment:10 Changed 11 years ago by jhpalmieri

Okay, that's good enough for me. Thanks for checking that.

comment:11 Changed 11 years ago by rlm

  • Merged in set to sage-4.5.alpha1
  • Milestone set to sage-4.5
  • Resolution set to fixed
  • Reviewers set to Nathann Cohen
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.