Skip to content
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

Implement solve_all for MiniZinc #43

Merged
merged 13 commits into from
Sep 22, 2023
Merged

Conversation

zengjian-hu-rai
Copy link
Contributor

@zengjian-hu-rai zengjian-hu-rai commented Sep 22, 2023

Add option num_solutions

  • num_solutions will be default to 1, unless it's set explicitly

  • Changed primal_solution to primal_solutions to hold 1+ solutions

  • Modified MOI.ResultCount, MOI.TerminationStatus.

  • If num_solutions > 1 and we've found the given number of solutions, return MOI.SOLUTION_LIMIT, otherwise MOI.OPTIMAL

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05% 🎉

Comparison is base (8574b83) 97.32% compared to head (4acf817) 97.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
+ Coverage   97.32%   97.37%   +0.05%     
==========================================
  Files           3        3              
  Lines         448      457       +9     
==========================================
+ Hits          436      445       +9     
  Misses         12       12              
Files Changed Coverage Δ
src/optimize.jl 92.90% <100.00%> (+0.48%) ⬆️

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

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Quite a few change suggestions, but all stemming from the high-level comment:

  • Let's just have one num_solutions option, store it in the options dictionary, and always add it to the command line args.

src/optimize.jl Outdated Show resolved Hide resolved
src/optimize.jl Outdated Show resolved Hide resolved
src/optimize.jl Outdated Show resolved Hide resolved
src/optimize.jl Outdated Show resolved Hide resolved
src/optimize.jl Outdated Show resolved Hide resolved
test/examples/nqueens_solveall.jl Outdated Show resolved Hide resolved
test/examples/nqueens_solveall.jl Outdated Show resolved Hide resolved
test/examples/nqueens_solveall.jl Outdated Show resolved Hide resolved
test/examples/nqueens_solveall.jl Outdated Show resolved Hide resolved
test/examples/nqueens_solveall.jl Outdated Show resolved Hide resolved
@zengjian-hu-rai
Copy link
Contributor Author

Quite a few change suggestions, but all stemming from the high-level comment:

  • Let's just have one num_solutions option, store it in the options dictionary, and always add it to the command line args.

Thanks for the great suggestions! Hopefully I've addressed them all.

src/optimize.jl Outdated Show resolved Hide resolved
src/optimize.jl Outdated Show resolved Hide resolved
Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

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

Okay, I added a bunch more changes. But this looks good now. Let me know if you're still happy and I'll merge.

@zengjian-hu-rai
Copy link
Contributor Author

Okay, I added a bunch more changes. But this looks good now. Let me know if you're still happy and I'll merge.

Yes it looks great! thanks so much for polishing!

@odow
Copy link
Member

odow commented Sep 22, 2023

thanks so much for polishing!

No problem. Sorry for being somewhat pedantic, but I hope you'll agree it looks a lot tidier now.

@odow odow merged commit 6cda1e7 into jump-dev:master Sep 22, 2023
7 checks passed
@zengjian-hu-rai
Copy link
Contributor Author

thanks so much for polishing!

No problem. Sorry for being somewhat pedantic, but I hope you'll agree it looks a lot tidier now.

Yes it does, thanks again.

@zengjian-hu-rai zengjian-hu-rai deleted the zhu-solve-all branch September 22, 2023 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants