Skip to content

propt: change interface for solving under assumptions #7976

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

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

kroening
Copy link
Member

@kroening kroening commented Oct 26, 2023

This changes the interface offered by propt for 'solving under assumptions' from a state-ful variant to the state-less API that MiniSat uses.

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@kroening kroening force-pushed the propt-interface branch 2 times, most recently from c34852e to 211e778 Compare October 26, 2023 12:04
Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

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

Seems reasonable and stateless does make things simpler. I wonder if there is a way to tweak the API so that you can also get back the set of necessary assumptions if it is UNSAT.

@kroening kroening force-pushed the propt-interface branch 2 times, most recently from b2ff37e to 9097886 Compare October 27, 2023 07:26
@kroening
Copy link
Member Author

kroening commented Oct 27, 2023

Seems reasonable and stateless does make things simpler. I wonder if there is a way to tweak the API so that you can also get back the set of necessary assumptions if it is UNSAT.

That would be nice -- I can't think of a nice way to do that without state however. Note we already have is_in_conflict(...).

This changes the interface offered by propt for 'solving under assumptions'
from a state-ful variant to the state-less API that MiniSat uses.
@kroening kroening marked this pull request as ready for review October 27, 2023 09:48
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (a010865) 78.45% compared to head (9508799) 78.06%.
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7976      +/-   ##
===========================================
- Coverage    78.45%   78.06%   -0.39%     
===========================================
  Files         1701     1701              
  Lines       196270   196325      +55     
===========================================
- Hits        153976   153258     -718     
- Misses       42294    43067     +773     
Files Coverage Δ
src/solvers/prop/prop.cpp 65.21% <100.00%> (+7.32%) ⬆️
src/solvers/prop/prop_conv_solver.cpp 87.35% <100.00%> (-0.15%) ⬇️
src/solvers/refinement/bv_refinement_loop.cpp 98.43% <100.00%> (ø)
src/solvers/sat/cnf_clause_list.h 80.00% <100.00%> (ø)
src/solvers/sat/external_sat.cpp 80.45% <100.00%> (ø)
src/solvers/sat/satcheck_minisat2.cpp 63.39% <100.00%> (+0.24%) ⬆️
src/solvers/sat/satcheck_minisat2.h 75.00% <100.00%> (ø)
unit/solvers/sat/satcheck_minisat2.cpp 100.00% <100.00%> (ø)
src/solvers/sat/dimacs_cnf.h 27.27% <0.00%> (ø)
src/solvers/sat/external_sat.h 0.00% <0.00%> (ø)
... and 2 more

... and 75 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +41 to +43
static const bvt empty_assumptions;
++number_of_solver_calls;
return do_prop_solve();
return do_prop_solve(empty_assumptions);
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about just using {} as an argument here instead of the static local?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that the static prevents the creation of a new object, and hence, would be a tiny bit more efficient. But that may well not be measurable.

@kroening kroening merged commit aed4d77 into develop Oct 27, 2023
@kroening kroening deleted the propt-interface branch October 27, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants