Fix MILP solver parameters: remove LP-tuned custom solver options - #357
Open
mokhtar-sidi wants to merge 1 commit into
Open
Fix MILP solver parameters: remove LP-tuned custom solver options#357mokhtar-sidi wants to merge 1 commit into
mokhtar-sidi wants to merge 1 commit into
Conversation
Removed the custom solver options for CPLEX and Gurobi in temoa/_internal/run_actions.py. All solvers now run with their default settings. The previous options were tuned for LPs and cause problems once the integer (MILP) extensions are activated: disabling crossover can crash MIP solves, and the loose barrier/feasibility tolerances are not appropriate for integer solves. Per the maintainers' recommendation, the safest fix is to drop these custom options and let each solver choose settings appropriate to the problem type (LP or MILP). The previous LP-tuned values (matching mip-dev / PyPSA) are kept as an inline reference comment so advanced users can re-tune LP performance if needed, e.g. via the extensions' *_solver_options.toml files. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Walkthrough
ChangesSolver configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@temoa/_internal/run_actions.py`:
- Around line 223-230: Remove the no-op solver-name conditional in the action
execution flow, including the cbc, cplex, and gurobi branches and the adjacent
appsi_highs no-op branch. Delete the entire conditional scaffolding rather than
preserving solver-specific branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be20b822-873c-4ed5-8756-a54b30b786fe
📒 Files selected for processing (1)
temoa/_internal/run_actions.py
Comment on lines
223
to
+230
| if solver_name == 'cbc': | ||
| pass | ||
|
|
||
| elif solver_name == 'cplex': | ||
| # Note: these parameter values match mip-dev / PyPSA | ||
| # (see: https://pypsa-eur.readthedocs.io/en/latest/configuration.html) | ||
| optimizer.options['lpmethod'] = 4 # barrier | ||
| optimizer.options['solutiontype'] = 2 # non basic solution, ie no crossover | ||
| optimizer.options['barrier convergetol'] = 1.0e-3 | ||
| optimizer.options['feasopt tolerance'] = 1.0e-4 | ||
| pass | ||
|
|
||
| elif solver_name == 'gurobi': | ||
| # Note: these parameter values match mip-dev / PyPSA (see: https://pypsa-eur.readthedocs.io/en/latest/configuration.html) | ||
| optimizer.options['Method'] = 2 # barrier | ||
| optimizer.options['Crossover'] = 0 # non basic solution, ie no crossover | ||
| optimizer.options['BarConvTol'] = 1.0e-3 | ||
| optimizer.options['FeasibilityTol'] = 1.0e-4 | ||
| optimizer.options['BarOrder'] = -1 # auto ordering; 2-4x faster than AMD on large models | ||
| pass |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the now-dead solver configuration branches.
All branches are no-ops; remove this conditional (including the adjacent unchanged appsi_highs no-op branch) rather than retaining solver-name-specific scaffolding.
Proposed cleanup
- if solver_name == 'cbc':
- pass
-
- elif solver_name == 'cplex':
- pass
-
- elif solver_name == 'gurobi':
- pass
-
- elif solver_name == 'appsi_highs':
- pass
-
# Suffix Handling📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if solver_name == 'cbc': | |
| pass | |
| elif solver_name == 'cplex': | |
| # Note: these parameter values match mip-dev / PyPSA | |
| # (see: https://pypsa-eur.readthedocs.io/en/latest/configuration.html) | |
| optimizer.options['lpmethod'] = 4 # barrier | |
| optimizer.options['solutiontype'] = 2 # non basic solution, ie no crossover | |
| optimizer.options['barrier convergetol'] = 1.0e-3 | |
| optimizer.options['feasopt tolerance'] = 1.0e-4 | |
| pass | |
| elif solver_name == 'gurobi': | |
| # Note: these parameter values match mip-dev / PyPSA (see: https://pypsa-eur.readthedocs.io/en/latest/configuration.html) | |
| optimizer.options['Method'] = 2 # barrier | |
| optimizer.options['Crossover'] = 0 # non basic solution, ie no crossover | |
| optimizer.options['BarConvTol'] = 1.0e-3 | |
| optimizer.options['FeasibilityTol'] = 1.0e-4 | |
| optimizer.options['BarOrder'] = -1 # auto ordering; 2-4x faster than AMD on large models | |
| pass |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 223-227: Combine if branches using logical or operator
Combine if branches
(SIM114)
[warning] 226-230: Combine if branches using logical or operator
Combine if branches
(SIM114)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@temoa/_internal/run_actions.py` around lines 223 - 230, Remove the no-op
solver-name conditional in the action execution flow, including the cbc, cplex,
and gurobi branches and the adjacent appsi_highs no-op branch. Delete the entire
conditional scaffolding rather than preserving solver-specific branches.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removed the custom solver options for CPLEX and Gurobi in temoa/_internal/run_actions.py. All solvers now run with their default settings.
The previous options were tuned for LPs and cause problems once the integer (MILP) extensions are activated: disabling crossover can crash MIP solves, and the loose barrier/feasibility tolerances are not appropriate for integer solves. Per the maintainers' recommendation, the safest fix is to drop these custom options and let each solver choose settings appropriate to the problem type (LP or MILP).
The previous LP-tuned values (matching mip-dev / PyPSA) are kept as an inline reference comment so advanced users can re-tune LP performance if needed, e.g. via the extensions' *_solver_options.toml files.
Summary by CodeRabbit