Make GDP enumeration lazy - #4031
Conversation
emma58
left a comment
emma58
left a comment
There was a problem hiding this comment.
Thank you for this--I agree it's a good change since someone could always want to cut off something unreasonable at a time or iteration limit. I think the implementation could be simpler though: There's no need to calculate the number of discrete solutions, we can just iterate through the generator until we hit a termination condition or are done.
| self.num_discrete_solns *= 2 ** len(non_indicator_boolean_vars) * math.prod( | ||
| max(0, index(v.ub) - index(v.lb) + 1) for v in discrete_vars |
There was a problem hiding this comment.
While it would make sense for the bounds of integer variables to be integer, Pyomo doesn't enforce that. Your call to index here will raise an error in that case. I think instead you should round correctly and cast the result to int.
| self.num_discrete_solns = len(discrete_solns) | ||
| for soln in discrete_solns: | ||
| # We will interrupt based on time limit or iteration limit: | ||
| for _ in range(self.num_discrete_solns): |
There was a problem hiding this comment.
On second thought, there's no reason to calculate num_discrete_solutions ever. We could just iterate through the solutions in self._discrete_solution_iterator here.
|
Thanks for the feedback. I agree that precomputing num_discrete_solns is unnecessary. I’ve updated the implementation to iterate directly over _discrete_solution_iterator, so the index calculation (and the bounds issue you identified) is removed entirely. I also removed the custom _log_current_state because its only difference from the base implementation was displaying iteration/num_discrete_solns. Without that count, the override would either reference a removed attribute or duplicate the base logger (and show AttributeError). |
55a60b9 to
eb44fbf
Compare
emma58
left a comment
emma58
left a comment
There was a problem hiding this comment.
This looks much better, thank you! On question though--I forgot that we overrode the progress logger to show progress towards complete enumeration. I think that may have been wise and we should add back in the calculation of how many solutions there are for that. Do you agree?
| # Override logging so that we print progress in terms of the number of | ||
| # iterations needed to fully enumerate the discrete space. | ||
| def _log_current_state(self, logger, subproblem_type, primal_improved=False): | ||
| star = "*" if primal_improved else "" | ||
| logger.info( | ||
| self.log_formatter.format( | ||
| "{}/{}".format(self.iteration, self.num_discrete_solns), | ||
| subproblem_type, | ||
| self.LB, | ||
| self.UB, | ||
| self.relative_gap(), | ||
| get_main_elapsed_time(self.timing), | ||
| star, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Oh, I see. Sorry, I wasn't being very careful. It may be worth keeping the calculations for how many discrete solutions there are so that unsuspecting users get this info in the log. Do you agree?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4031 +/- ##
=======================================
Coverage 89.94% 89.94%
=======================================
Files 917 917
Lines 109261 109255 -6
=======================================
- Hits 98276 98272 -4
+ Misses 10985 10983 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I think the cleanest approach is to keep iterating directly over _discrete_solution_iterator, while calculating num_discrete_solns separately only for the progress logger (which I would keep). The count would be computed arithmetically without materializing any solutions, and would use ceil(lb) and floor(ub) for integer-variable bounds. Does that look right to you? |
|
@alvaroborras, yes, I like that plan! |
emma58
left a comment
emma58
left a comment
There was a problem hiding this comment.
This looks good, thank you! On comment below because I realized we don't correctly handle unbounded discrete variables. That's not this PR's fault, but is worth fixing before we merge if you're willing.
| self.num_discrete_solns *= 2 ** len(non_indicator_boolean_vars) * math.prod( | ||
| max(0, math.floor(v.ub) - math.ceil(v.lb) + 1) for v in discrete_vars | ||
| ) |
There was a problem hiding this comment.
Sorry, this isn't actually a bug introduced by this PR, but I just caught it: add_discrete_variable_list doesn't check for bounds, so this will error if v.lb or v.ub is None. I think we should check that case here because this is the first place we'll catch it. It will also be a problem in _discrete_solution_iterator.
3 participants
Fixes #3953
Summary/Motivation:
gdpopt.enumeratepreviously materialized every discrete solution before checking the time and iteration limits. For large discrete spaces, this could exhaust available memory before solving began.This change counts the discrete solution space without materializing it and requests solutions lazily after checking the configured limits. It preserves correct completion behavior and supports reusing the solver instance.
Changes proposed in this PR:
AI-Use Disclosure
or
AI tools contributed to the development of this PR
Review process (select ONE):
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: