Skip to content

BnB: Add ability to specify integers#17

Open
tuncbkose wants to merge 4 commits into
engri-1101:masterfrom
tuncbkose:master
Open

BnB: Add ability to specify integers#17
tuncbkose wants to merge 4 commits into
engri-1101:masterfrom
tuncbkose:master

Conversation

@tuncbkose

@tuncbkose tuncbkose commented Jun 20, 2024

Copy link
Copy Markdown

@henryrobbins Thanks for your response in #16. Here is what had put together, though I don't have a quick example to test (beyond the included tests).

I'm still not sure about what is happening in visualize.py, but I may take a look at it later. Meanwhile, I'd appreciate your comments on this.

One question about the slack variables: In the original code is it correct to do if np.sum(frac_comp) > 0: once slack variables are added? Could this not lead to a case where you are branching on a slack variable, which is not necessarily correct?

Todo:

  • Adjust LP to support specifying integrality
  • Change simplex and branch_and_bound to support integrality
  • Fix tests broken by the above
  • Refactor bnb_visual
  • Add more tests for MILPs

@tuncbkose tuncbkose marked this pull request as ready for review June 20, 2024 08:40

@henryrobbins henryrobbins left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry for the huge delay in reviewing this! Thanks for putting it together!

Comment thread gilp/simplex.py Outdated
Comment thread gilp/simplex.py Outdated
Comment thread gilp/simplex.py
@henryrobbins

Copy link
Copy Markdown
Collaborator

I pulled your PR locally and tried it out on a 2D and 3D example. Seems to work!

Screenshot 2024-07-17 at 12 00 26 AM

@henryrobbins henryrobbins self-assigned this Jul 17, 2024
@henryrobbins henryrobbins added the enhancement New feature or request label Jul 17, 2024
@henryrobbins henryrobbins linked an issue Jul 17, 2024 that may be closed by this pull request
@tuncbkose

Copy link
Copy Markdown
Author

Thanks for the review! And no worries about the delay. I'll try to add more to this PR whenever I have the time.

@henryrobbins

Copy link
Copy Markdown
Collaborator

I still don't know if/what implications these may have for the visualization code because I couldn't get around to looking at it yet.

The branch_and_bound implementation and bnb_visual both call branch_and_bound_iteration. bnb_visual requires a lot of information from each iteration (current node, which constraints were added, remaining feasible regions, etc...). Because branch_and_bound only returned the final solution, it wasn't used in bnb_visual. That is resulting in a lot of duplicate code between branch_and_bound and bnb_visual. This should be refactored. One possibility is to include a "path" of the algorithm in the object returned by branch_and_bound that exposes all the necessary information for plotting (as is done with the simplex implementation. I'm open to other suggestions though.

@tuncbkose

Copy link
Copy Markdown
Author

I renamed int_mask to integrality. At the moment visualization is probably broken because it is calling simplex on the MILP rather than the relaxation. I'll leave fixing that to bnb_visual refactoring.

@tuncbkose

Copy link
Copy Markdown
Author

It's not a rewrite but I did add .get_relaxation() to all the simplex calls in bnb_visual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ENH: Generalizing the branch and bound implementation

2 participants