8000 feat: Add `pumpkin_solver` solver by lmmx · Pull Request #96 · rust-or/good_lp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Add pumpkin_solver solver #96

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lmmx
Copy link
@lmmx lmmx commented Mar 16, 2025
  • feat(pumpkin-solver): add Pumpkin solver integration to Cargo features and solvers
  • fix(temporary): unsure how to set objective bounds properly, hotfix [-1000, +1000]

I’m not sure about a couple of things here:

  • Could I get feedback on whether I’ve added the conditional compilation directives correctly in lib.rs?
  • I have temporarily set bounds of [-1000, +1000] but obviously this needs to be done properly

TODO

  • Remove solve once assertion (ownership model precludes reuse) 0adc5c6
  • Add to CI 3dd3330
  • Add to README
  • Fix CI test failures
    • default solver not exported (here) - fixed in a550c97
      • TODO: cargo test --no-default-features --features pumpkin-solver should pass
  • Add more tests
  • Work out how to set bounds properly in objective var (remove [-1000, +1000] hotfix)

* feat(pumpkin-solver): add Pumpkin solver integration to Cargo features and solvers

* build(pumpkin): add conditional compilation directives

* fix: typo

* build: cargo build success 🎉

* build: cargo builds again

* fix: cargo builds 🎉 (2 unused warnings)

* fix: new try

* fix: solve simple problem test pass

* test: solve easy and solve milp passing 🎉

* fix: tests were passing for the wrong reasons

* fix(temporary): unsure how to set objective bounds properly, hotfix [-1000, +1000]
@lmmx lmmx changed the title feat: Add pumpkin_solver solver (#1) feat: Add pumpkin_solver solver Mar 16, 2025
Copy link
Collaborator
@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

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

The overall approach looks good to me. You may want to add some more tests, and update the README.

8000

@lovasoa
Copy link
Collaborator
lovasoa commented Mar 17, 2025

You also need to update the CI to test the new solver: https://github.com/rust-or/good_lp/blob/main/.github/workflows/rust.yml

@lovasoa
Copy link
Collaborator
lovasoa commented Mar 17, 2025

https://github.com/rust-or/good_lp/blob/main/CONTRIBUTING.md

@lmmx
Copy link
Author
lmmx commented Mar 17, 2025

Thanks a lot, I added pumpkin-solver* to the table with a no on the WASM box but I am not actually sure if that is something that needs special support or if you get it out of the box via good_lp

  • edit I tried and there was a specific requirement for Unix handlers so I can confirm WASM is not available (I imagine a workaround could be to stub out a workspace crate for the signal hooks crate providing SIGINT but it's better to fix upstream) WebAssembly compatible? ConSol-Lab/Pumpkin#161

*I feel like pumpkin would be a nicer feature name but I don’t know if it’s preferable to keep it exactly the same as the crate name…

I’ll try using it with existing good_lp code and see if I can road test it this week, add those missing tests and try to find a solution to the objective var bounds

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.

2 participants
0