engri-1101 / gilp

A Python package for visualizing the geometry of linear programs.
https://gilp.henryrobbins.com
Other
46 stars 8 forks source link

BnB: Add ability to specify integers #17

Open tuncbkose opened 2 months ago

tuncbkose commented 2 months ago

@henryrobbins Thanks for your response in https://github.com/engri-1101/gilp/issues/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:

henryrobbins commented 1 month ago

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

tuncbkose commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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