-
Notifications
You must be signed in to change notification settings - Fork 45
Autogapper #518
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
Autogapper #518
Conversation
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.
Pull Request Overview
This PR implements the new “Autogapper” feature that enhances mipgap handling via configurable options and unified extension handling. Key changes include:
- Adding an optional name prefix to gapper configuration in mpisppy/utils/config.py.
- Introducing the add_gapper function in mpisppy/utils/cfg_vanilla.py and updating fixer handling.
- Replacing separate iteration counters with _PHIter across several cylinder modules and updating the mipgapper extension for improved option checking and logging.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mpisppy/utils/config.py | Modified gapper_args to add an optional name parameter for configurable gapper options. |
| mpisppy/utils/cfg_vanilla.py | Added add_gapper to configure gapper options and adjusted fixer options based on verbose value. |
| mpisppy/generic_cylinders.py | Updated gapper_args calls and integrated add_gapper, plus unified iteration counter usage. |
| mpisppy/extensions/mipgapper.py | Revised mipgapper init, option checking, and logging with global_toc; now forces verbose. |
| mpisppy/cylinders/subgradient_bounder.py | Replaced the dk_iter counter with _PHIter to maintain iteration consistency. |
| mpisppy/cylinders/spoke.py | Enhanced send_bound to conditionally update inner/outer bounds and provide error feedback. |
| mpisppy/cylinders/spcommunicator.py | Introduced compute_gaps to calculate absolute and relative gaps based on current bounds. |
| mpisppy/cylinders/lagrangian_bounder.py | Updated iteration counter handling to use _PHIter. |
| mpisppy/cylinders/hub.py | Removed the redundant compute_gaps method, deferring gap calculations to the spcommunicator. |
mpisppy/extensions/mipgapper.py
Outdated
| if self.verbose and self.cylinder_rank == 0: | ||
| print ("(rank0) mipgapper:" + str) | ||
| or self.gapperoptions.get("verbose", True) | ||
| self.verbose = True |
Copilot
AI
May 13, 2025
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.
Verbose output is being forced to True, which ignores any configuration provided via self.ph.options or gapperoptions. Consider removing or modifying this hardcoding to allow configurable verbosity.
| self.verbose = True |
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.
Going to eliminate the verbose flag for the extension and unconditionally print the gap change
bknueven
left a comment
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.
Need to add a blurb in the documentation about the new gapper options
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.
Pull Request Overview
Adds an “autogapper” mode to the existing MIP gap scheduler by extending configuration, wiring in an automatic gapper extension, and updating cylinders and docs to support dynamic subproblem gap adjustments.
- Expanded
gapper_argsto register both JSON‐based and automatic (starting+ratio) modes. - Introduced
add_gapperhelper incfg_vanilla.pyand replaced ad‐hoc wiring ingeneric_cylinders.py. - Implemented
Gapperextension with_autoset_mipgap, updated spokes/communicator to compute problem gaps, and refreshed docs.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mpisppy/utils/config.py | gapper_args now takes optional name and new options |
| mpisppy/utils/cfg_vanilla.py | Added add_gapper and JSON loading for gap schedules |
| mpisppy/generic_cylinders.py | Replaced manual gapper wiring with vanilla.add_gapper |
| mpisppy/extensions/mipgapper.py | New automatic‐mode logic and gap‐setting methods |
| mpisppy/cylinders/subgradient_bounder.py | Initialize and increment _PHIter for subgradient spoke |
| mpisppy/cylinders/lagrangian_bounder.py | Initialize and increment _PHIter for lagrangian spoke |
| mpisppy/cylinders/spoke.py | send_bound now updates BestInnerBound/BestOuterBound |
| mpisppy/cylinders/spcommunicator.py | Added compute_gaps to calculate abs/rel problem gaps |
| mpisppy/cylinders/hub.py | Removed redundant compute_gaps override |
| doc/src/extensions.rst | Documented automatic gapper options |
Comments suppressed due to low confidence (1)
mpisppy/extensions/mipgapper.py:75
- [nitpick] Consider adding unit tests for the new automatic gapper logic, especially the branch where
PHIter == 1and the dynamic gap adjustment in_autoset_mipgap.
elif self.ph._PHIter == 1:
No description provided.