-
Notifications
You must be signed in to change notification settings - Fork 557
Gurobi solution pool tweak #3726
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
base: main
Are you sure you want to change the base?
Conversation
Is this a my code issue or an issue with win/3.9 and 3.13? |
@viens-code - neither were related to your code. The 3.9 one was a spurious failure; the 3.13 one is now just happening everywhere (yay broken infrastructure). I'm looking into it. |
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.
One requested change for a test, but more fundamentally, why is this needed? Can't users get exactly the same behavior with
gurobi_generate_solutions(..., solver_options={'PoolSearchMode': 2})
try: | ||
gurobi_generate_solutions(m, pool_search_mode=0) | ||
except AssertionError as e: | ||
pass |
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.
This is a weak test: not only will any assertion that fails pass, but not raising any error at all will also pass. The recommended / standard way to test an error is to check both the error type and the message:
with self.assertRaisesRegex(AssertionError, "pool_search_mode must be 1 or 2"):
gurobi_generate_solutions(m, pool_search_mode=0)
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.
Changed over to the suggested format
First, will make the tweaks to the testing logic to update to your suggestion, thank you for that. To the larger point, there are a couple of things going on with the alternative_solutions package. Yes an advanced user could change from enforced enumerate (mode 2) to best effort (mode 1) with the manual flag if they knew about it. It also parallels how things like num_solutions are handled as parameters outside solver_options. |
@viens-code as you're working on additional changes, please make sure to merge main into your branch. This will resolve the win/3.13 failure which is coming from a testing infrastructure issue. |
Using assertRaisesRegex format now
Changes to that test structure done |
@viens-code seems like it was a temporary CI issue. Re-running the win/3.10 job seems to have resolved it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3726 +/- ##
==========================================
+ Coverage 89.27% 89.32% +0.04%
==========================================
Files 896 896
Lines 103687 103695 +8
==========================================
+ Hits 92567 92622 +55
+ Misses 11120 11073 -47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I agree with Matthew's argument that the pool_search_mode provides a convenient API simplification that will make it easier for end-users. |
Changed to the more general paradigm for handling solver options for best effort mode alternative solution generation in gurob_generate_solutions. Added comments explaining the functionality and how to use
Tweaks made to handling the solver options as requested in code review. |
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.
This is getting close... In addition to @blnicho's bugfix, some additional quick changes would be good:
Added logging tests with LoggingIntercept. Added assertation check for num_solutions = 0 case Removed unneccessary skipIf numpy unavailable from tests that didn't use numpy
…sion/pyomo-aos into gurobi_solution_pool_tweak
Ok, all of the changes should be made now. Edit: And of course I forget to apply black. Now fixed |
Pyomo Spell checking breaks if you use the word assertation anywhere. It apparently will tolerate assertion but not assertation. Now using assert instead of assertation
Spelling Change. The word 'assertation' breaks the spellchecker |
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.
There are a couple lines that are too long (the strings constants should be split), but overall, this is fine.
Fixes #3725
Summary/Motivation:
Adds the ability to do best effort solution pool collection
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: