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. |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
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.
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.
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.31% 89.32%
=======================================
Files 896 896
Lines 103687 103695 +8
=======================================
+ Hits 92610 92622 +12
+ Misses 11077 11073 -4
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. |
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
|
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 |
|
@jsiirola I'll fix the long strings as part of the follow-up solution pool PR |
6 participants
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: