-
Notifications
You must be signed in to change notification settings - Fork 16
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
pull request for Sat backend #415
base: main
Are you sure you want to change the base?
pull request for Sat backend #415
Conversation
solvers/sat_rs/src/main.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what your longer term intentions are, but if this just wraps rustsat, it might not need to be its own crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: you won't be able to use any conjure_core types in here due to circular dependencies, so any conversion code from Conjure Oxide AST will have to go in conjure_core not a seperate crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not to create a wrapper for rustsat, I'm trying to pretty much replace the kissat_rs crate that we're using - mostly because this will provide the functionality for all or most sat solvers. I'm essentially just writing functions to create a tree and to solve that tree so that the larger conjure_core/conjure_oxide functions can call. I'm not planning on calling any of the conjure_core functions from this so, that's not something we need to worry about. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Note that we never actually used kissat_rs
, so I wouldn't worry about trying to match that in any way. (more generally, as long as things still conform to the solver interface, you can gut and rework them as much as you want)
Code and Documentation Coverage ReportDocumentation CoverageClick to view documentation coverage for this PR
Click to view documentation coverage for main
Code Coverage SummaryThis PR: Detailed Report
Main: Detailed Report
Coverage Main & PR Coverage ChangeLines coverage changed by -0.50% and covered lines changed by -20
Functions coverage changed by -0.20% and covered lines changed by -1
Branches... coverage: No comparison data available |
Not sure what the plan is here @Shikhar-Srivastava-16 - are you planning to update the PR to work with the solver adaptor interface or abandon this and work on a separate PR or soemthing else? |
I would be happy to run through my slides on the solver adaptor / do a bit of Q&A during Wednesdays meeting if it would be helpful @ozgurakgun @Shikhar-Srivastava-16 @ewilbert7 |
great idea, thanks for the offer. |
That would very helpful! Thanks for the offer. |
Added relevant functions for the loading step, the only remaining todo is to fix the error handling - at the moment the function uses the The adaptor is set up to use a standard instance of the solve structs in |
…solve function rmoved kissat file **currently using MiniSat
@@ -26,6 +26,9 @@ schemars = "0.8.21" | |||
clap = { version = "4.5.20", features = ["derive"] } | |||
itertools = "0.13.0" | |||
im = "15.1.0" | |||
sat_rs = { version = "0.1.0", path = "../../solvers/sat_rs" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we decided to remove the sat_rs crate, is it still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still being used for some of the model loading step, so retaining it.
3abe23a
to
55d0458
Compare
c7cf943
to
c2b2d32
Compare
Opened a PR for the sat backend, very basic implementation at the moment but as this point, the best approach is to work with the rest of the team. Please take a look at the code to see if there are any glaring problems