Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing a bug where the CPLEX check was forcefully implemented even when using Gurobi #96

Merged
merged 4 commits into from Oct 13, 2023

Conversation

tanubrata
Copy link
Collaborator

Description:

  • Addresses the issue where CPLEX check was ran even with Gurobi being used

Changes:

  • added independent checks so that if gurobi is loaded, it will not run CPLEX check.
  • if Cplex is present, but not gurobi, it will run CPLEX check for gGnome. If both gurobi and cplex is present, it will still run CPLEX check because by default JaBbA uses CPLEX unless otherwise mentioned gurobi.

Impact:

  • Should address the issue where users were facing issues installing JaBbA when gurobi was being used as optimizer instead of CPLEX.

Copy link
Collaborator

@shihabdider shihabdider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just make the small change I suggested and we can merge.

R/JaBbA.R Outdated
@@ -55,13 +55,28 @@ low.count=high.count=seg=chromosome=alpha_high=alpha_low=beta_high=beta_low=pred
jmessage("${CPLEX_DIR}/cplex/[(include)|(lib)] do not both exist")
}

if (!requireNamespace("gurobi", quietly = TRUE)) {
jmessage("Gurobi is not installed! REMEMBER: You need to have either CPLEX or Gurobi!")
if (((!file.exists(paste0(cplex.dir, "/cplex"))) & (!file.exists(paste0(cplex.dir, "/cplex/include")) ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to store these booleans into variables with informative names (e.g cplex_exists, gurobi_exists etc.). This is will make these conditionals much easier to read and reduce clutter (since you're repeating this code several times for subsequent conditionals).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok done! I have updated my JaBbA td_dev.

Copy link
Collaborator

@shihabdider shihabdider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better! Please change _installation to _installed (you should try to approach natural english as much as you can when making these kinds of readability changes--if gurobi_installed sounds more natural than if gurobi_installation). Also remove the explanatory comments. Comments should really only be used to explain why you did something, not what you did. Let code itself show what you did.

Also use git commit --amend to avoid adding a new commit.

@tanubrata
Copy link
Collaborator Author

Done!

@shihabdider shihabdider merged commit e75f3cb into mskilab-org:master Oct 13, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants