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

solve! returns the problem #658

Merged
merged 1 commit into from
May 14, 2024
Merged

Conversation

ericphanson
Copy link
Collaborator

Currently it returns nothing. I think this is nicer since then you get the solution summary printed after you do solve!, along with a summary of the problem. It's possible this can help users notice issues if they weren't looking at the problem, but did solve! in the REPL and noticed the output. In addition, looking at some of the examples like https://jump.dev/Convex.jl/dev/examples/mixed_integer/n_queens/, this would add more details to the example without needing a separate example block to show the problem first.

There is currently an explicit test that we return nothing, so I traced back the origin of that test to see why (in case there's some particular reason we need to return nothing). I believe it was added in #62 originally, and was carried forward in various formats since then (eventually in its own testset once we added the problem depot since handle_problem there doesn't necessarily call solve!). Looking at that PR, I don't think returning nothing was necessarily important for anything, it was just testing what we were currently doing then.

@ericphanson ericphanson requested a review from odow May 14, 2024 15:50
Copy link

codecov bot commented May 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.88%. Comparing base (f8a56c4) to head (d448390).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #658   +/-   ##
=======================================
  Coverage   97.88%   97.88%           
=======================================
  Files          89       89           
  Lines        5205     5205           
=======================================
  Hits         5095     5095           
  Misses        110      110           

☔ 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.

LGTM.

@ericphanson ericphanson merged commit 68e9efc into master May 14, 2024
10 checks passed
@ericphanson ericphanson deleted the eph/solve-returns-the-problem branch May 14, 2024 20:13
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