search

saezlab / CARNIVAL

CAusal Reasoning for Network Identification with integer VALue programming in R
https://saezlab.github.io/CARNIVAL/
57 stars 29 forks source link

Added gurobi option to solver #30

Closed djinnome closed 3 years ago

djinnome commented 4 years ago

I have successfully run gurobi on the LP file that CARNIVAL generates (although the 255 character limit in gurobi required me to rename a couple of the variables), and then I went ahead and extended CARNIVAL with gurobi functionality. Now if solver="gurobi" then it will create a gurobi Command file with the appropriate gurobi parameters, and it will generate a result file similar to CBC output that is parsed by exportResultGurobi.R. I created a pull request, but before you accept, I wanted to discuss the possibility of a general fix for the 255 character limit on the variables that gurobi chokes on.

ivanovaos commented 4 years ago

Hi @djinnome, thanks for your contribution. Can you please point out where the issue with 255 limit can arise, do you have any specific example in mind?

We aim to follow a style guide in the next versions. Can you please make sure that your code follows this style guide http://jef.works/R-style-guide/?

djinnome commented 4 years ago

Hi @ivanovaos I would be happy to adhere to any style guidelines you follow.

With regard to an example: In https://github.com/saezlab/covid19/runCarnival.md the attached lp file that was generated contains a couple of very long variable names at each of the indicated line numbers:

291427 c291423: xb13712_1 - xb20593_1 + B_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3_1 - xb6831_1 = 0       

298307 c298303: B_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3_1 = 0      

349467 c349464: 101 xb71752_1 + dist_MAPT - dist_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3 <= 100      

403613 c403610: 101 xb125898_1 + dist_MAPT - dist_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3 <= 100     

542417  -1 <= B_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3_1 <= 1

549298  0 <= dist_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3 <= 100

556229  0 <= dist_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3 <= 100

692047  B_MT_ND1_MT_ND2_MT_ND3_MT_ND4_MT_ND5_MT_ND6_NDUFA1_NDUFA10_NDUFA11_NDUFA12_NDUFA13_NDUFA2_NDUFA3_NDUFA4_NDUFA5_NDUFA6_NDUFA7_NDUFA8_NDUFA9_NDUFAB1_NDUFB1_NDUFB10_NDUFB11_NDUFB2_NDUFB3_NDUFB4_NDUFB5_NDUFB6_NDUFB7_NDUFB8_NDUFB9_NDUFC1_NDUFC2_NDUFS1_NDUFS2_NDUFS3_NDUFS4_NDUFS5_NDUFS6_NDUFS7_NDUFS8_NDUFV1_NDUFV2_NDUFV3_1
djinnome commented 4 years ago

Here is the attached lp file: testFile_1_1.lp

djinnome commented 4 years ago

Also, gurobi chokes on the nonstandard header: "enter Problem\n\n" in the first 2 lines of the lp file.

This is due to lines 15-16 in writeSolverFiles.R: https://github.com/saezlab/CARNIVAL/blob/85bafc3be250398a70f34aeab82ef6d9584e6b7f/R/writeSolverFiles.R#L15-L16

djinnome commented 4 years ago

Since we are speaking of style guidelines, I was a bit worried when I saw this nested if...else statement:

https://github.com/saezlab/CARNIVAL/blob/85bafc3be250398a70f34aeab82ef6d9584e6b7f/R/solveCARNIVALSingle.R#L94-L96

I am fairly new to R, so I thought this might be a limitation of the language. Fortunately, according to https://www.datamentor.io/r-programming/if-else-statement/ there does exist an if...else ladder.

Better to do

  } else if(solver=="cbc"){
djinnome commented 4 years ago

One potential improvement to this pull request is to factor out the common code between exportResultCBC.R and exportResultGurobi.R as the only difference between them is the way they parse their respective solution files:

exportResultCBC.R:

  solMatrix = read_csv(cplexSolutionFileName)

vs exportResultGurobi.R:

  solMatrix = read_delim(cplexSolutionFileName, " ", col_names=c("variable","value") )
ivanovaos commented 3 years ago

@djinnome branch renaming lead to accidental closing of this pull request. Can you please re-do the request to devel_2_1? Thank you!